PR_GetPhysicalMemorySize returns garbage on systems with more than 2GB RAM

RESOLVED FIXED in 4.6.1

Status

NSPR
NSPR
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({fixed1.8})

other
4.6.1
x86
Linux
fixed1.8
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
PR_GetPhysicalMemorySize returns garbage on systems with more than 2GB RAM

I observed this with 32-bit x86-linux on a machine with 3GB RAM.

PR_GetPhysicalMemorySize returns 18446744072724996096.  The problem is with this
code:

    long pageSize = sysconf(_SC_PAGESIZE);
    long pageCount = sysconf(_SC_PHYS_PAGES);
    LL_I2L(bytes, pageSize * pageCount);

sizeof(long) is 4, so on my system "pageSize * pageCount" is a negative value. 
LL_I2L casts the negative value to a PRInt64, which ends up producing the bogus
value.

This bug ends up causing Firefox to think that it has a zero-sized memory cache
on my system, which obviously degrades performance.

I'll attach a candidate patch shortly.
(Assignee)

Comment 1

12 years ago
Created attachment 196105 [details] [diff] [review]
v1 patch

Here's a simple patch that solves the problem.	It's interesting that LL_UI2L
is actually identical to LL_I2L, so it is not enough to change the macro.  I
need the cast to force the correct result.  I decided to change the macro for
clarity only.
Attachment #196105 - Flags: review?(wtchang)
(Assignee)

Comment 2

12 years ago
I think it is crucial that this fix go into Firefox 1.5.
Status: NEW → ASSIGNED
Flags: blocking1.8b5?
Target Milestone: --- → 4.6.1
(Assignee)

Updated

12 years ago
Blocks: 295074

Comment 3

12 years ago
Comment on attachment 196105 [details] [diff] [review]
v1 patch

> #if defined(LINUX) || defined(SOLARIS)
> 
>     long pageSize = sysconf(_SC_PAGESIZE);
>     long pageCount = sysconf(_SC_PHYS_PAGES);
>-    LL_I2L(bytes, pageSize * pageCount);
>+    LL_UI2L(bytes, (unsigned long) pageSize * pageCount);
> 
> #elif defined(HPUX)

I think it's better to declare pageSize and pageCount
as PRUint64:

      PRUint64 pageSize, pageCount;
      LL_I2L(pageSize, sysconf(_SC_PAGESIZE));
      LL_I2L(pageCount, sysconf(_SC_PHYS_PAGES));
      LL_MUL(bytes, pageSize, pageCount);

This is for the case of a 32-bit OS running on a system
with more than 4GB RAM.

Could you test this?
Attachment #196105 - Flags: review?(wtchang)
(Assignee)

Comment 4

12 years ago
Yeah, I like that better.  I will try it now.
(Assignee)

Comment 5

12 years ago
Wan-Teh: Yeah, that patch works fine.  However, on platforms that do not have a
native 64-bit integer type, we would end up evaluating sysconf twice per LL_I2L
invocation.  So, temporary |long| variables are warranted.

Comment 6

12 years ago
Sigh.  You can just use regular arithmetic operators to
write that code.

    PRUint64 pageSize = sysconf(_SC_PAGESIZE);
    PRUint64 pageCount = sysconf(_SC_PHYS_PAGES);
    bytes = pageSize * pageCount;

It's 2005 now.  No need to use the LL_ macros.

Comment 7

12 years ago
I agree with Wan-Teh. We should not support compilers that don't offer a long
long type anymore. I believe there are already several places in NSS which break
those compilers, and no one has been complaining.

Comment 8

12 years ago
arg. please don't. some of us plan to deal with those edge cases.

Comment 9

12 years ago
Comment on attachment 196105 [details] [diff] [review]
v1 patch

We should also review the implementations for the other
platforms to see if they have the same overflow problem.

The code for HPUX also multiplies two 32-bit numbers
(in 32-bit HP-UX), so it needs to be fixed the same way.
(The type of info.physical_memory and info.page_size
is _T_LONG_T, which is either int32_t or int64_t depending
on whether the HP-UX OS is 32-bit or 64-bit.)  The code for
other platforms looks fine.

In the code for AIX, we have
    LL_MUL(bytes, kbytes, 1024);
It is wrong to use a constant in the LL_MUL macro.

timeless: please name a compiler you need to support that
doesn't have a 64-bit integer type (which doesn't need
to be called "long long").  All the compilers NSPR support
today have a 64-bit integer type.  Our portability guidelines
need to be reviewed from time to time, and I think it is
safe to say goodbye to the LL_ macros now.
(Assignee)

Comment 10

12 years ago
Wan-Teh: Ok, I'll take a crack at putting together a better patch shortly.
(Assignee)

Comment 11

12 years ago
Created attachment 196269 [details] [diff] [review]
v2 patch

Let me know if you want a more minimal patch.  I just decided to remove the LL_
macros from all #ifdef'd sections.
Attachment #196105 - Attachment is obsolete: true
Attachment #196269 - Flags: review?(wtchang)

Comment 12

12 years ago
Comment on attachment 196269 [details] [diff] [review]
v2 patch

You missed this LL_ macro:

>     PRUint64 bytes = LL_ZERO;

In the following two changes:

>-    LL_I2L(bytes, pageSize * pageCount);
>+    bytes = (PRUint64) pageSize * pageCount;

and

>-        LL_I2L(bytes, info.physical_memory * info.page_size);
>+        bytes = (PRUint64) info.physical_memory * info.page_size;

I assume the (PRUint64) applies to the first operand rather
than the product (i.e., the typecast binds more tightly than
the * operator), and you rely on the automatic promotion of
the second operand to PRUint64.  Correct?
(Assignee)

Comment 13

12 years ago
> You missed this LL_ macro:
> 
> >     PRUint64 bytes = LL_ZERO;

Yeah, that was intentional.  I only changed code inside the #ifdef's, but I'll
go ahead and change that as well.


> I assume the (PRUint64) applies to the first operand rather
> than the product (i.e., the typecast binds more tightly than
> the * operator), and you rely on the automatic promotion of
> the second operand to PRUint64.  Correct?

Yes, exactly.  If you prefer, I will make the casting more explicit.
(Assignee)

Comment 14

12 years ago
Created attachment 196273 [details] [diff] [review]
v2.1 patch

BTW, I tested this patch on Linux-x86 (3 GB RAM), OSX-ppc (1 GB RAM), and WinXP
(1 GB RAM).
Attachment #196269 - Attachment is obsolete: true
Attachment #196273 - Flags: review?(wtchang)
(Assignee)

Updated

12 years ago
Attachment #196269 - Flags: review?(wtchang)

Updated

12 years ago
Attachment #196273 - Flags: review?(wtchang) → review+

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+
(Assignee)

Comment 15

12 years ago
Comment on attachment 196273 [details] [diff] [review]
v2.1 patch

I think we should fix this for Firefox 1.5
Attachment #196273 - Flags: approval1.8b5?
(Assignee)

Comment 16

12 years ago
fixed on NSPR trunk and NSPRPUB_PRE_4_2_CLIENT_BRANCH.

I assume that we will want to also land this on the following branches:
MOZILLA_1_8_BRANCH and NSPR_4_6_BRANCH.

I will await explicit approval before doing so.

Comment 17

12 years ago
Yes, MOZILLA_1_8_BRANCH and NSPR_4_6_BRANCH should
be kept in sync for as long as possible.

Updated

12 years ago
Attachment #196273 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Comment 18

12 years ago
fixed1.8, fixed-nspr4.6
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.