442012 - Allocating more than 4GB using nsMemory::Alloc(size_t) succeeds but less memory is allocated

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Michal Novotny [:michal]]

Type size_t is unsigned long int on 64-bit OS. So it is possible to ask for more than 4GB of memory using nsMemory::Alloc(size_t). But nsMemory::Alloc(size_t) calls NS_Alloc(PRSize) which calls PR_Malloc(PRUint32) where the most significant half of the parameter is thrown away. E.g. allocating 5GB of memory will succeed but only 1GB will be allocated.

Comment 1

[Daniel Veditz [:dveditz]]

Benjamin: assigning to you to figure out what approach we take. Change the nsMemory::Alloc() signature, or change the backend implementation to work correctly on 64-bit systems?

Surely NSPR has run into this problem, what do they do on 64-bit systems?

Comment 2

[Wan-Teh Chang]

The NSPR bug for this issue is bug 445295.

You can change NS_Alloc(PRSize) to call malloc(size_t) instead
of PR_Malloc(PRUint32).  Remember to call calloc and free instead
of PR_Calloc and PR_Free in NS_Realloc and NS_Free.

Comment 3

[Wan-Teh Chang]

That should have been "Remember to call realloc and free instead
of PR_Realloc and PR_Free in NS_Realloc and NS_Free."

Comment 4

[Benjamin Smedberg]

What size is PRSize on 64-bit systems?

Comment 5

[Michal Novotny [:michal]]

(In reply to comment #4)
> What size is PRSize on 64-bit systems?

(gdb) whatis PRSize
type = size_t
(gdb) whatis size_t
type = long unsigned int
(gdb) p sizeof(PRSize)
$1 = 8

Comment 6

[Benjamin Smedberg]

Attachment: Prevent allocations >2GB, rev. 1

Let's just bail on allocations >2GB... I can't think of any reason they should be allowed.

Comment 7

[Wan-Teh Chang]

Comment on attachment 345539 [details] [diff] [review]
Prevent allocations >2GB, rev. 1

Don't forget to patch NS_Realloc, too.

Comment 8

[Benjamin Smedberg]

Attachment: Prevent allocations >2GB, rev. 1.1

Comment 9

[Wan-Teh Chang]

Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

r=wtc.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

sr=dveditz

Comment 11

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/9f3807b5e936

Comment 12

[Benjamin Smedberg]

Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

Assuming the perf metrics aren't affected, this should be very safe.

Comment 13

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

Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

a191=beltzner, didn't see any perf impact after landing.

Comment 14

[Benjamin Smedberg]

Fixed on 1.9.1, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5b26e181c6d5

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 16

[Benjamin Smedberg]

Landed on CVS HEAD for 1.9.0.6

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Is there anything that really needs to be done to verified that this is fixed in 1.9.0.6?

Comment 18

[Benjamin Smedberg]

I don't think you could verify it with a 32-bit OS. Since there's no testcase here, I think it can safely be left alone.

Comment 19

[Alexander Sack]

seems we want it on 1.8 too.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

Approved for 1.8.1.21, a=dveditz for release-drivers.

Comment 21

[Alexander Sack]

fixed for 1.8.1.21

Checking in xpcom/base/nsMemoryImpl.cpp;
/cvsroot/mozilla/xpcom/base/nsMemoryImpl.cpp,v  <--  nsMemoryImpl.cpp
new revision: 1.29.4.2; previous revision: 1.29.4.1

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 version

Comment 23

[Alexander Sack]

Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

a=asac for 1.8.0