Closed Bug 1180195 Opened 4 years ago Closed 4 years ago

Uninitialised value use in Predictor::SpaceCleaner::OnMetaDataElement

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

Attachments

(2 files)

When loading http://tangerine-tycoon.wikia.com/wiki/Tangerine_Tycoon_Wiki
(note, the Tangerine Tycoon may be a Red Herring; this may happen more widely?)
I have Valgrind complaining of undefinedness in 

netwerk/base/Predictor.cpp:1485
  if (!ok || !mKeyToDelete || lastHit < mLRUStamp) {
Attached file Valgrind complaint
I spent some time trying to make sense of this.  It's not entirely
straightforwards.  Here's my best guess as to the sequence of events:

::OnMetaDataElement is called on some object

we have stack-allocated |lastHit| being uninitialised
1480  uint32_t hitCount, lastHit, flags;

ParseMetaDataEntry is called, and fails, returning |false| and not
setting |lastHit|
1481  bool ok = mPredictor->ParseMetaDataEntry(key, value, [..]

Because |ok| == false, this is taken
1484  if (!ok || !mKeyToDelete || lastHit < mLRUStamp) {

and we copy the undefined value of |lastHit| into |mLRUStamp|
1486    mLRUStamp = lastHit;

Some time later, ::OnMetaDataElement is called on the same object.
This time, the call to ParseMetaDataEntry, and |mKeyToDelete| is
non-null, so the conditional becomes

1484  if (false || false || lastHit < mLRUStamp) { ..
In this case, |lastHit| is defined, because ParseMetaDataEntry
succeeded (returned |true|), but |mLRUStamp| contains the
garbage value we set it to in in the previous call.



I verified, by reading the machine code, that it is indeed the
"lastHit < mLRUStamp" part of the conditional that Valgrind is
complaining about.

It seems pretty strange to set mLRUStamp to a garbage value when
the ParseMetaData call fails -- is that intended?
Attached patch A possible fixSplinter Review
Attachment #8629362 - Flags: review?(hurley)
Comment on attachment 8629362 [details] [diff] [review]
A possible fix

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

Yeah, this looks like the right fix. Thanks!
Attachment #8629362 - Flags: review?(hurley) → review+
https://hg.mozilla.org/mozilla-central/rev/d299c67518bf
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.