Closed Bug 472032 Opened 12 years ago Closed 12 years ago

[win64] sizeof(long) != sizeof(void*) assertion in nsScriptSecurityManager.cpp

Categories

(Core :: Security: CAPS, defect)

x86_64
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Mook, Assigned: Mook)

References

()

Details

Attachments

(2 files, 1 obsolete file)

See URL.  This assertion gets triggered because sizeof(long) = 4, sizeof(void*) = 8.

See bug 119646 comment 9 to comment 20.  It's guarding union SecurityLevel at http://mxr.mozilla.org/mozilla-central/source/caps/include/nsScriptSecurityManager.h?rev=9f497b1505d2&mark=149#144

This triggers on startup of any win64 build.  I have no idea how the other win64 people manage to not see it...

###!!! ASSERTION: long and void* have different lengths on this platform. This may cause a security failure.: 'sizeof(long) == sizeof(void*)', file .../mozilla/src/caps/src/nsScriptSecurityManager.cpp, line 3220
insert of asserting, just use the type that's guaranteed to be sizeof(void*) instead.

Thanks to timeless for helping tracking down the initial reason.
Attachment #363527 - Flags: superreview?(dveditz)
Attachment #363527 - Flags: review?(dveditz)
Attachment #363527 - Flags: superreview?(dveditz)
Attachment #363527 - Flags: superreview+
Attachment #363527 - Flags: review?(dveditz)
Attachment #363527 - Flags: review+
Comment on attachment 363527 [details] [diff] [review]
use PRWord, not long, and drop assertion

r/sr=dveditz

Would it be better to leave the assertion and change it to sizeof(PRWord) instead? It seems to be the right thing to use here (jst and sicking agree) but I'm slightly concerned by the comment in prtypes.h saying it's deprecated. Updating the assertion might help if they stop maintaining it for future platforms. If you do leave the assertion change the text to end "This may cause a security failure with the SecurityLevel union."
carrying forward r/sr

(In reply to comment #3)
> (From update of attachment 363527 [details] [diff] [review])

> Would it be better to leave the assertion and change it to sizeof(PRWord)
> instead? 
Done.

> I'm slightly concerned by the comment in prtypes.h saying it's deprecated.
Unfortunately, unless NSPR decides to provide a type that actually is guaranteed to be sizeof(void*), we're stuck with it.  C99 got intptr_t, so having such a type must make sense to somebody :)
Assignee: dveditz → mook.moz+mozbz
Attachment #363527 - Attachment is obsolete: true
Attachment #364282 - Flags: superreview+
Attachment #364282 - Flags: review+
Keywords: checkin-needed
(In reply to comment #4)
> Unfortunately, unless NSPR decides to provide a type that actually is
> guaranteed to be sizeof(void*)

See PRUptrdiff.
Comment on attachment 364282 [details] [diff] [review]
address comment 3
[Checkin: Comment 6]


http://hg.mozilla.org/mozilla-central/rev/04579855a2b7
Attachment #364282 - Attachment description: address comment 3 → address comment 3 [Checkin: Comment 5]
Attachment #364282 - Attachment description: address comment 3 [Checkin: Comment 5] → address comment 3 [Checkin: Comment 6]
(In reply to comment #5)
> See PRUptrdiff.

Ah well: reopen as needed.
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attached patch PRUptrdiffSplinter Review
Bah, sorry, I thought I had already attached this, then my internet access went down :(

Here's a trivial version to use PRUptrdiff, if you think it's necessary?  (Feel free to clear the requests out if not.)

For what it's worth, I used PRWord because it's not a pointer difference (as in, intptr_t vs ptrdiff_t)
Attachment #366299 - Flags: superreview?(peterv)
Attachment #366299 - Flags: review?(peterv)
Attachment #366299 - Flags: superreview?(peterv)
Attachment #366299 - Flags: superreview+
Attachment #366299 - Flags: review?(peterv)
Attachment #366299 - Flags: review+
Comment on attachment 366299 [details] [diff] [review]
PRUptrdiff

>diff --git a/caps/include/nsScriptSecurityManager.h b/caps/include/nsScriptSecurityManager.h

>+    PRUptrdiff level;
>+    char*     capability;

Nit: alignment is not right.

>+    NS_ASSERTION(sizeof(PRUptrdiff) == sizeof(void*),

Make this a PR_STATIC_ASSERT.
You need to log in before you can comment on or make changes to this bug.