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

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: michal, Assigned: bsmedberg)

Tracking

({fixed1.8.1.21, fixed1.9.0.6, fixed1.9.1})

Trunk
x86
Linux
fixed1.8.1.21, fixed1.9.0.6, fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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.
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?
Assignee: nobody → benjamin

Comment 2

9 years ago
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

9 years ago
That should have been "Remember to call realloc and free instead
of PR_Realloc and PR_Free in NS_Realloc and NS_Free."
(Assignee)

Comment 4

9 years ago
What size is PRSize on 64-bit systems?
(Reporter)

Comment 5

9 years ago
(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
(Assignee)

Comment 6

9 years ago
Created attachment 345539 [details] [diff] [review]
Prevent allocations >2GB, rev. 1

Let's just bail on allocations >2GB... I can't think of any reason they should be allowed.
Attachment #345539 - Flags: superreview?(dveditz)
Attachment #345539 - Flags: review?(dveditz)

Comment 7

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

Don't forget to patch NS_Realloc, too.
Attachment #345539 - Flags: review+
(Assignee)

Comment 8

9 years ago
Created attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1
Attachment #345539 - Attachment is obsolete: true
Attachment #346730 - Flags: superreview?(dveditz)
Attachment #346730 - Flags: review+
Attachment #345539 - Flags: superreview?(dveditz)
Attachment #345539 - Flags: review?(dveditz)

Comment 9

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

r=wtc.
Attachment #346730 - Flags: review+
Attachment #346730 - Flags: superreview?(dveditz) → superreview+
Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

sr=dveditz
(Assignee)

Comment 11

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9f3807b5e936
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

9 years ago
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.
Attachment #346730 - Flags: approval1.9.1?
Attachment #346730 - Flags: approval1.9.0.6?
Flags: wanted1.9.0.x+
Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

a191=beltzner, didn't see any perf impact after landing.
Attachment #346730 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 1.9.1 landing]
Whiteboard: [needs 1.9.1 landing] → [sg:low][needs 1.9.1 landing]
(Assignee)

Comment 14

9 years ago
Fixed on 1.9.1, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5b26e181c6d5
Keywords: fixed1.9.1
Whiteboard: [sg:low][needs 1.9.1 landing] → [sg:low]
Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #346730 - Flags: approval1.9.0.6? → approval1.9.0.6+
(Assignee)

Comment 16

8 years ago
Landed on CVS HEAD for 1.9.0.6
Keywords: fixed1.9.0.6
Is there anything that really needs to be done to verified that this is fixed in 1.9.0.6?
(Assignee)

Comment 18

8 years ago
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

8 years ago
seems we want it on 1.8 too.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next+

Updated

8 years ago
Attachment #346730 - Flags: approval1.8.1.next?
Attachment #346730 - Flags: approval1.8.0.next?
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment on attachment 346730 [details] [diff] [review]
Prevent allocations >2GB, rev. 1.1

Approved for 1.8.1.21, a=dveditz for release-drivers.
Attachment #346730 - Flags: approval1.8.1.next? → approval1.8.1.next+

Comment 21

8 years ago
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
Keywords: fixed1.8.1.21

Comment 22

8 years ago
Created attachment 359027 [details] [diff] [review]
1.8.0 version
Group: core-security

Comment 23

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

a=asac for 1.8.0
Attachment #346730 - Flags: approval1.8.0.next? → approval1.8.0.next+
You need to log in before you can comment on or make changes to this bug.