1180195 - Uninitialised value use in Predictor::SpaceCleaner::OnMetaDataElement

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Julian Seward [:jseward]]

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) {

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaint

Comment 2

[Julian Seward [:jseward]]

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?

Comment 3

[Julian Seward [:jseward]]

Attachment: A possible fix

Comment 4

[u408661]

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!

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d299c67518bf

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d299c67518bf