Checking the wrong value for < 0 when testing a preference value

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

unspecified
mozilla35
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed)

Details

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #1038897 +++

From https://bugzilla.mozilla.org/show_bug.cgi?id=1038897#c13:

Comment on attachment 8458795 [details] [diff] [review] [diff] [review]
pop entries off the cache until its size reaches max cache size. Input read from corresponding pref is clamped to non-negative values

Review of attachment 8458795 [details] [diff] [review] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgLoader.cpp
@@ +1030,5 @@
>    if (NS_SUCCEEDED(rv))
>      sCacheMaxSize = cachesize;
>    else
>      sCacheMaxSize = 5 * 1024 * 1024;
> +  sCacheMaxSize = sCacheMaxSize > 0 ? sCacheMaxSize : 0;

Isn't it already too late?  In the success case above, we assigned int32_t cachesize to uint32_t sCacheMaxSize.  I think the code should have been:

if (NS_SUCCEEDED(rv))
  sCacheMaxSize = cachemaxsize > 0 ? cachemaxsize : 0;
else
  sCacheMaxSize = 5 * 1024 * 1024;

and then there is no need for the sCacheMaxSize = sCacheMaxSize > 0 ? sCacheMaxSize : 0 at all (which really doesn't do anything for uint32_t anyway.)
Seth, I'll leave it to you to decide if this needs an uplift, based on the original bug 1038897 (if it applies.)
Assignee: nobody → milan
Comment on attachment 8491772 [details] [diff] [review]
Check the signed value for < 0, too late once we cast it to unsigned.

Review of attachment 8491772 [details] [diff] [review]:
-----------------------------------------------------------------

That's a great point! Thanks Milan.

(BTW, should be r=seth)
Attachment #8491772 - Flags: review?(seth) → review+
(In reply to Milan Sreckovic [:milan] from comment #2)
> Seth, I'll leave it to you to decide if this needs an uplift, based on the
> original bug 1038897 (if it applies.)

Given how low-risk this patch is, I think it's worth uplifting, since the original patch hasn't hit release yet. However, the risk to users is admittedly low.
Comment on attachment 8491772 [details] [diff] [review]
Check the signed value for < 0, too late once we cast it to unsigned.

Approval Request Comment
[Feature/regressing bug #]: 1038897
[User impact if declined]: Setting a negative value for "image.cache.size" could cause unexpected behavior because the existing code handles conversion between signed and unsigned incorrectly.
[Describe test coverage new/current, TBPL]: None right now.
[Risks and why]: The risk here is *incredibly* low, so even though the number of users who'd be affected if we didn't uplift is probably very small, I think this is probably worth uplifting just so the bad code doesn't end up in a release.
[String/UUID change made/needed]: None.
Attachment #8491772 - Flags: approval-mozilla-beta?
Attachment #8491772 - Flags: approval-mozilla-aurora?
Summary: Checking the wrong value for < 0 when testing the → Checking the wrong value for < 0 when testing a preference value
https://hg.mozilla.org/mozilla-central/rev/6a918435e41c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Attachment #8491772 - Flags: approval-mozilla-beta?
Attachment #8491772 - Flags: approval-mozilla-beta+
Attachment #8491772 - Flags: approval-mozilla-aurora?
Attachment #8491772 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.