Closed Bug 560298 Opened 10 years ago Closed 10 years ago

npapi.h forces illegal (truncated) pointer casts on Windows x64

Categories

(Core :: Plug-ins, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .9-fixed

People

(Reporter: vanboxem.ruben, Assigned: jaas)

References

Details

(Keywords: 64bit, arch)

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; nl; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/releases/3.6.3/source/firefox-3.6.3.source.tar.bz2

Using mingw-w64 from mingw-w64.sourceforge.net, while building WebKit, using npapi.h, which I believe is maintained here.

The _NPEvent definition is not compatible with x64 Windows. Due to LPARAM and WPARAM being 64-bits long, the declarations on lines 481-482:
478 typedef struct _NPEvent
479 {
480   uint16_t event;
481   uint32_t wParam;
482   uint32_t lParam;
483 } NPEvent;
This together with (lines 3203-3257, specifically 3249-3250):
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp
and the type of aMsg is MSG, wich has a WPARAM and LPARAM, which are 64-bit on x64 Windows: http://msdn.microsoft.com/en-us/library/aa384242%28v=VS.9%29.aspx. This is bad code. 
Please change lines 481 and 482 to:
uintptr_t lParam;
uintptr_t rParam;
This maintains 32-bit compatibility and ensures 64-bit correctness.
Thank you.

link to WebKit bug report:
https://bugs.webkit.org/show_bug.cgi?id=35607



Reproducible: Always

Steps to Reproduce:
1.Download mingw-w64 from sourceforge and set up like mingw.org's gcc
2.compile any file that tries to put a pointer or reference into a _NPEvent's wparam.
3. for example:
#include <windows.h>
#include <stdio.h>
#include "npapi.h"
int main()
{
    _NPEvent event;
    WINDOWPOS pos;
    printf( "Size of msg.wParam is : %i\n", sizeof(&pos) );
    printf( "Size of event.wParam is : %i", sizeof(event.wParam) );
    event.wParam = reinterpret_cast<uint32>(&pos);
}
This is analogous to what happens in WebKit's code.

Actual Results:  
Compiling with MSVC with /Wp64 produces pointer truncation warning:
cl main.cpp /wp64
Compiling with mingw-w64 produces pointer precision loss error:
g++ main.cpp

Expected Results:  
When uintptr_t is used, the problem goes away, transparently and in a portable way.
Summary: npapi.h forces illegal casts on Windows x64 + GCC → npapi.h forces illegal (truncated) pointer casts on Windows x64
Keywords: 64bit, arch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → joshmoz
Attached patch fix v1.0Splinter Review
Attachment #441285 - Flags: superreview?(jst)
Attachment #441285 - Flags: superreview?(jst) → superreview+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/b6cb47c9c4b8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Many thanks for the fast resolution!
No problem, thanks for bringing this to our attention.

Google and Apple should be taking this fix as well. You should probably make a WebKit bug specifically for this issue so that it gets done ASAP and isn't tied to the more general bug you reported it in.
Well, too late for a specific bug report. It's already been landed as part of something larger in webkit trunk. The changelog does specify this bug report as the origin of the change.

WebKit trunk commit: http://trac.webkit.org/changeset/58256
Where are you expecting MSVC to declare uintptr_t? Mine has it in stddef.h but nptypes.h doesn't include that.
npapi.h has this:
    53 #ifdef _WINDOWS
    54 # include <windef.h>
    55 # ifndef XP_WIN
    56 #  define XP_WIN 1
    57 # endif /* XP_WIN */
    58 #endif /* _WINDOWS */
Which is more than OK for (u)intptr_t I think. nptypes.h shouldn't need it.
Neil - are you actually having problems compiling as a result of this change?
Ah, so windef.h includes winnt.h includes ctype.h that includes crtdefs.h that includes vadefs.h that defines uintptr_t.
Comment on attachment 441285 [details] [diff] [review]
fix v1.0

We should take this on 1.9.2.5 to avoid any confusion. It is safe.
Attachment #441285 - Flags: approval1.9.2.5?
Comment on attachment 441285 [details] [diff] [review]
fix v1.0

We will not be taking this for 3.6.6. Moving approval request forward.

If you disagree, send me an email.
Attachment #441285 - Flags: approval1.9.2.7?
Attachment #441285 - Flags: approval1.9.2.6-
Attachment #441285 - Flags: approval1.9.2.5?
Comment on attachment 441285 [details] [diff] [review]
fix v1.0

Approved for 1.9.2.9, a=dveditz for release-drivers
Attachment #441285 - Flags: approval1.9.2.9? → approval1.9.2.9+
You need to log in before you can comment on or make changes to this bug.