Last Comment Bug 759417 - MapsMemoryReporter asserts with device #s > 100
: MapsMemoryReporter asserts with device #s > 100
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: mozilla15
Assigned To: Nathan Froyd [:froydnj]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-29 11:16 PDT by Nathan Froyd [:froydnj]
Modified: 2012-06-02 12:31 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to handle 3-digit device numbers (1.53 KB, patch)
2012-05-29 11:16 PDT, Nathan Froyd [:froydnj]
justin.lebar+bug: review-
Details | Diff | Splinter Review
revised patch (1.95 KB, patch)
2012-05-30 12:12 PDT, Nathan Froyd [:froydnj]
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-05-29 11:16:22 PDT
Created attachment 628025 [details] [diff] [review]
patch to handle 3-digit device numbers

My xpcshell test runs are failing locally with the error:

###!!! ASSERTION: Didn't create a vsize node?: 'categoriesSeen.mSeenVsize', file /home/froydnj/src/m-c.git/xpcom/base/MapsMemoryReporter.cpp, line 237

Poking around with GDB reveals that /proc/self/smaps has an unusual header:

00400000-0044e000 r-xp 00000000 103:00 27190725 <other stuff that FILE's members didn't have>

Note the 3-digit major device number, which MapsMemoryReporter is unprepared to handle.

Apparently it's not terribly uncommon:

@nightcrawler:/opt/build/m-c-git-build$ egrep -c '[0-9]{3}:' /proc/*/maps 2>/dev/null | cut -f 2 -d ':' | addup
111

out of some 4k+ map entries.  This an Ubuntu 11.04 box.

The attached patch appears to solve the problem (only gets xpcshell running properly, haven't looked at about:memory or anything).  The *scanf %[ construct scans a range of characters until the field width is hit or a character not in the range is found.  I chose to keep a field width for safety's sake; perhaps there are four-digit device numbers lurking somewhere...
Comment 1 Justin Lebar (not reading bugmail) 2012-05-29 16:23:28 PDT
Comment on attachment 628025 [details] [diff] [review]
patch to handle 3-digit device numbers

r=me.

Do we have any reason to believe 3 digits is the right length?  2 digits was a complete guess...
Comment 2 Nicholas Nethercote [:njn] 2012-05-29 16:41:59 PDT
Wait!  The numbers are hex, not decimal.  From the example in "man 5 proc":

    08048000-08056000 r-xp 00000000 03:0c 64593   /usr/sbin/gpm

Please fix that!  And the example uses lower-case letters but I'd use [0-9A-Fa-f] as the pattern just to be safe.

I looked at Valgrind's code for parsing these files.  It just looks for an arbitrary length hex number.  How about we allow 4 or even 5 digits, just in case?
Comment 3 Nathan Froyd [:froydnj] 2012-05-29 16:48:54 PDT
(In reply to Nicholas Nethercote [:njn] from comment #2)
> Wait!  The numbers are hex, not decimal.  From the example in "man 5 proc":

Aha!  That squares with what I see; the major device number of my raid array is 259.

> Please fix that!  And the example uses lower-case letters but I'd use
> [0-9A-Fa-f] as the pattern just to be safe.

Will do.

> I looked at Valgrind's code for parsing these files.  It just looks for an
> arbitrary length hex number.  How about we allow 4 or even 5 digits, just in
> case?

_Linux Device Drivers_ indicates that the 2.6 kernel allocates 12 bits to the major device number and 20 bits to the minor device number, so I'll use that as a guideline for field widths.
Comment 4 Justin Lebar (not reading bugmail) 2012-05-29 21:29:41 PDT
> Wait!

I guess I shouldn't review patches during MemShrink meetings, huh?
Comment 5 Nathan Froyd [:froydnj] 2012-05-30 12:12:32 PDT
Created attachment 628420 [details] [diff] [review]
revised patch

New patch!
Comment 6 Justin Lebar (not reading bugmail) 2012-05-30 12:21:54 PDT
Comment on attachment 628420 [details] [diff] [review]
revised patch

I'd be OK if you wanted to make these |char dev[32]| just so we don't have to worry about relying on an internal kernel data structure -- we don't even use the dev major/minor values for anything in about:memory.  But I'm happy to do this too.
Comment 7 Nathan Froyd [:froydnj] 2012-05-30 12:39:25 PDT
(In reply to Justin Lebar [:jlebar] from comment #6)
> I'd be OK if you wanted to make these |char dev[32]| just so we don't have
> to worry about relying on an internal kernel data structure -- we don't even
> use the dev major/minor values for anything in about:memory.  But I'm happy
> to do this too.

More conservative values would be good.  I'll just use |char dev[17]|; 64 bits should be enough for anybody.

Note You need to log in before you can comment on or make changes to this bug.