PR_GetPhysicalMemorySize returns garbage on systems with more than 2GB RAM

RESOLVED FIXED in 4.6.1

Status

defect
--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: darin.moz, Assigned: darin.moz)

Tracking

({fixed1.8})

Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted patch v1 patch (obsolete) — Splinter Review
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)
I think it is crucial that this fix go into Firefox 1.5.
Status: NEW → ASSIGNED
Flags: blocking1.8b5?
Target Milestone: --- → 4.6.1
Blocks: 295074
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)
Yeah, I like that better.  I will try it now.
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.
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.
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.
arg. please don't. some of us plan to deal with those edge cases.
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.
Wan-Teh: Ok, I'll take a crack at putting together a better patch shortly.
Posted patch v2 patch (obsolete) — Splinter Review
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 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?
> 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.
Posted patch v2.1 patchSplinter Review
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)
Attachment #196269 - Flags: review?(wtchang)
Attachment #196273 - Flags: review?(wtchang) → review+
Flags: blocking1.8b5? → blocking1.8b5+
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?
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.
Yes, MOZILLA_1_8_BRANCH and NSPR_4_6_BRANCH should
be kept in sync for as long as possible.
Attachment #196273 - Flags: approval1.8b5? → approval1.8b5+
fixed1.8, fixed-nspr4.6
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.