759417 - MapsMemoryReporter asserts with device #s > 100

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: 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)]

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 [inactive]]

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]]

(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)]

> Wait!

I guess I shouldn't review patches during MemShrink meetings, huh?

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: revised patch

New patch!

Comment 6

[Justin Lebar (not reading bugmail)]

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]]

(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.

Comment 8

[Nathan Froyd [:froydnj]]

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

Comment 9

[(no longer active)]

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