733977 - Linux and OSX startup crash in Pickle::ReadUInt32

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Andrew McCreight [:mccr8]]

Only ranked 40, but given that it is mostly Linux that seems fairly high. 91% Linux, 9% OSX. 100% of crashes are after < 1 min. I'm guessing it could be a regression from bug 707320.

In the ones I was looking at, the pickle function is being called from LoadHistogramEvent::Run.

Here are some examples:
https://crash-stats.mozilla.com/report/index/240f7645-18c9-48d8-8499-4ad4b2120306
https://crash-stats.mozilla.com/report/index/5d1b91b2-0f62-45ad-9570-fcaa52120306

Comment 1

[Scoobidiver (away)]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> I'm guessing it could be a regression from bug 707320.
Not sure because there are crashes in 12.0a2/20120207 and 20120209.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Scoobidiver from comment #1)
> (In reply to Andrew McCreight [:mccr8] from comment #0)
> > I'm guessing it could be a regression from bug 707320.
> Not sure because there are crashes in 12.0a2/20120207 and 20120209.

We don't care about any crash earlier than 2012029, because it's not the same code.

Comment 3

[Nathan Froyd [:froydnj]]

OK, so I can reproduce this on my machine by doing something like:

echo 'fghk' > $PROFILE_DIRECTORY/sessionHistograms.dat.8little
firefox -P $PROFILE_NAME -new-instance

It looks like we need to check that the amount of file we read in roughly agrees with how much data Pickle thinks it has to read.  I'm not sure I can do that without getting my hands dirty with internals of Pickle.

This looks like it would take care of bug 722625 as well.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: patch with more sanity-checking

Here's a patch for checking:

1) That the file is actually big enough to contain the minimum amount of data we need.  This wasn't actually causing the crash, but it is a latent bug.  (If we had 0-byte data files, then we would crash a little bit earlier than where this crash report is about.)

2) That the amount of data stored in the pickle is synonymous with how much data we know we read.  This check is an early check for too-short data files, but it also catches the cases where:

- We only wrote the pickle header, but didn't write anything else.  It therefore looks like there's a lot of data to read, but there's actually not.

- The pickle header is badly corrupt.

(I'm not totally happy with this check, as it can assert in debug builds with suitably corrupt datafiles, but I suppose that is the point of debug builds, yes?)

Comment 5

[(dormant account)]

Comment on attachment 604108 [details] [diff] [review]
patch with more sanity-checking

depressing that pickle doesn't do this. More and more reason to rewrite this code :(

Comment 6

[(dormant account)]

Comment on attachment 604108 [details] [diff] [review]
patch with more sanity-checking

-  if (size == -1) {
+  if ((size_t)size < sizeof(Pickle::Header)) {

^-- seems bad, same sort of bad int->uint casting below.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: patch with more sanity-checking

Now with C++-style casts and a little more thought put into how to deal with signed-unsigned comparisons.

Comment 8

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 604513
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=470be8b24966
Try run started, revision 470be8b24966. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=470be8b24966

Comment 9

[Mozilla RelEng Bot]

Try run for 470be8b24966 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=470be8b24966
Results (out of 216 total builds):
    success: 179
    warnings: 36
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-470be8b24966

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9949b7477858

Comment 11

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/mozilla-central/rev/9949b7477858