514112 - MAX_DSLOTS_LENGTH is *net*, and avoid *size variable names for net lengths or capacities

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Karl Tomlinson (:karlt)]

With gcc -Wtype-limits:

/home/karl/moz/dev/js/src/jsarray.cpp: In function 'JSBool ResizeSlots(JSContext*, JSObject*, uint32, uint32)':
/home/karl/moz/dev/js/src/jsarray.cpp:327: warning: comparison is always false due to limited range of data type

If size == ~(uint32)0, size + 1 will overflow, and if realloc(ptr, 0) chooses
to return non-NULL, the overflow will not be detected.

Comment 1

[Karl Tomlinson (:karlt)]

I don't actually understand the comment for MAX_DSLOTS_LENGTH.  For ILP32, the
value looks like the maximum allocatable size *including* the hidden slot.  I
read bug 489899 comment 13, but that didn't make it seem right.
(At least ResizeSlots() seems to test against the right value for ILP32 systems,
though.)

http://hg.mozilla.org/mozilla-central/annotate/c07eb0b8b0f1/js/src/jsobj.h#l246

For LP64 systems, MAX_DSLOTS_LENGTH looks like a maximum possible total length
that could be passed to realloc, but ResizeSlots() will never pass a length
anything close to this.

Comment 2

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/annotate/b722d9fdf154/js/src/jsarray.cpp#l327

Comment 3

[Brendan Eich [:brendan]]

Yikes, who reviewed gal's patch? :-P

/*
 * Maximum net gross capacity of the obj->dslots vector, excluding the additional
 * hidden slot used to store the length of the vector.
 */
#define MAX_DSLOTS_LENGTH   (JS_MAX(~(uint32)0, ~(size_t)0) / sizeof(jsval))

"net gross" is an oxymoron. I'm a moron for r+'ing, though.

Karl, thanks for reporting this.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: MAX_DSLOTS_LENGTH is *net*, and avoid *size variable names for net lengths or capacities

Comment 5

[Andreas Gal :gal]

Comment on attachment 398081 [details] [diff] [review]
MAX_DSLOTS_LENGTH is *net*, and avoid *size variable names for net lengths or capacities

2nd time is the charm.

Comment 6

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/ec19fb045976

I'm usually a nut about "length" or "count" vs. (byte) "size", and net vs. gross. I shall redouble my efforts...

/be

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 398081 [details] [diff] [review]
MAX_DSLOTS_LENGTH is *net*, and avoid *size variable names for net lengths or capacities

This makes things much easier to understand and deals with most of the issues in comment 1, thanks, but doesn't address comment 0.

Consider either

js_NewUninitializedArray(,, len = 0xFFFFFFFF):
  ResizeSlots(,,, newcap = 0xFFFFFFFF);

or

EnsureCapacity(,, newcap = 0xFFFFFFFF):
  newcap = JS_ROUNDUP(0, CAPACITY_CHUNK) - 1;
  ResizeSlots(,,, newcap = 0xFFFFFFFF);

on LP64 systems such that MAX_DSLOTS_LENGTH = ~(uint64)0 / sizeof(jsval) - 1;
and so 0xFFFFFFFF < MAX_DSLOTS_LENGTH.

ResizeSlots(,,, newcap = 0xFFFFFFFF):
  newslots = cx->realloc(slots, bytes = 0);

http://hg.mozilla.org/mozilla-central/annotate/c07eb0b8b0f1/js/src/jsutil.h#l195

js_realloc(slots, bytes = 0)
  bytes = sizeof(void*);
  return realloc(slots, 8);

=> newslots has 8 bytes set aside for 0xFFFFFFFF slots.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: accurate MAX_DSLOTS_LENGTH when uint32(-1) < size_t(-1)

How about this?

Comment 9

[Brendan Eich [:brendan]]

Argh, I ignored all the stinking uint32 parameters. Could we not fix this other than by your patch using JS_MIN and uint32(-1), if we just used size_t instead of uint32 for the relevant newlen, etc., params?

/be

Comment 10

[Karl Tomlinson (:karlt)]

Yes.  uint32 -> size_t makes sense to me.

Comment 11

[Karl Tomlinson (:karlt)]

One thing to consider though is whether a minimal patch will be necessary for 1.9.1.

Comment 12

[Karl Tomlinson (:karlt)]

And another is that uint32(-1) * sizeof is already 32GB of memory so allowing for more elements may be early planning for the future.

Comment 13

[Karl Tomlinson (:karlt)]

... sizeof(jsval) ...

Comment 14

[Brendan Eich [:brendan]]

I am sucking at this -- back to 64-bit school for me. Andreas, can I tag out of the ring and let you hit this with a folding chair? I was your review wrestling partner on the regressing patch, but my bad-review guilt has run out :-P.

/be

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/ec19fb045976

Comment 16

[Karl Tomlinson (:karlt)]

Filed bug 517644 on this.

Comment 17

[Reed Loden [:reed]]

So, do we have a patch yet that is 1.9.1 branch worthy?

Comment 18

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/be0689de1b9f

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 398107 [details] [diff] [review]
accurate MAX_DSLOTS_LENGTH when uint32(-1) < size_t(-1)

The overflow was fixed in a different way in bug 517644.  MAX_DSLOTS_LENGTH is not used on LP64 systems except in compile-time tests to see whether it is greater than MAX_DSLOTS_LENGTH32.

Comment 20

[Samuel Sidler (old account; do not CC)]

Does the patch that landed apply to 1.9.1? If so, can you please request approval. If not, please attach one that does. Code freeze is scheduled for November 10 at 11:59pm PDT.

Comment 21

[Samuel Sidler (old account; do not CC)]

Gal? Karl?

Comment 22

[Karl Tomlinson (:karlt)]

I don't actually understand what this fixes.

The overflow reported in comment 0 was fixed in bug 517644.

Comment 23

[Karl Tomlinson (:karlt)]

... fixed on trunk, i mean.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

64 bit Linux isn't a supported platform for Gecko 1.9.1. Drivers would accept a nominated patch (looks like it would be easy) if someone can figure out which of these applies to that branch.