New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 860690 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Histogram APIs and truncation

Project Member Reported by dtapu...@chromium.org, Jul 6

Issue description

To better identify coding errors I'm iterating on a patch where "-Wconversion" compiler flag is applied in blink.

This identifies narrowing error from 64 bit values to 32 bit values and it has identified a few histograms which have issues:

Specifically consider this example:

  size_t a = 0xbaadbeef00000010LL;
  histogram->AddCount(20, a);

You might expect that this adds a count of the max value a histogram can support (which is INT_MAX - 1) but it will actually add a count of 0x10 because of the implicit narrowing.

The correct approach here would be to:

  size_t a = 0xbaadbeef00000010LL;
  histogram->AddCount(20, saturated_cast<int>(a));

But this is problematic to actually get correct everywhere as some people might just do a static_cast<int> which would be incorrect.

More problematic usages of the API are also seen with the ScaledLinearHistogram which lead the user to think that the domain of the value is scaled and then logged. But the value is first truncated and then scaled.

Problematic usages are seen in various spots:

https://chromium.googlesource.com/chromium/src/+/bf0112123d055721209dfe0c0f24b5774918ea9e/components/data_use_measurement/core/data_use_measurement.cc#438

This code uses a size_t member and scales it by 1024; which conceptually a consumer of this API would expect that values larger than 2gb could be counted but it can't because of the truncation. As well the truncating cast vs the saturated cast can cause incorrect counts.


 
Thanks for finding this.

Do you have a list of histograms that are passing 64 bit ints into these methods?

I agree that having people call saturated_cast isn't ideal as they would have to know to use it. 

Is there a way to enforce 32bit ints are passed to our methods? I suppose that is sort of what you are already doing since you're trying to add -Wconversion.

Is it a reasonable plan to fix all outstanding issues and rely on -Wconversion at some point soon(?) to enforce?

I also wonder if we can make some changes to the scaling logic so larger values can get passed in and we only check after the scaling is done - but I havn't looked enough to see how reasonable that kind of change would be.



I won't have an entire list until I fix all the errors in blink. There are many histograms probably >30 in blink alone. But there are thousands of narrowing errors in blink so I don't know how long it will take.

So are you indicating that each callsite should be switched to use saturated_cast? 

Sign in to add a comment