Closed
Bug 560298
Opened 14 years ago
Closed 14 years ago
npapi.h forces illegal (truncated) pointer casts on Windows x64
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(status1.9.2 .9-fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | .9-fixed |
People
(Reporter: vanboxem.ruben, Assigned: jaas)
References
Details
(Keywords: 64bit, arch)
Attachments
(1 file)
577 bytes,
patch
|
roc
:
superreview+
christian
:
approval1.9.2.7-
dveditz
:
approval1.9.2.9+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•14 years ago
|
Summary: npapi.h forces illegal casts on Windows x64 + GCC → npapi.h forces illegal (truncated) pointer casts on Windows x64
Reporter | ||
Updated•14 years ago
|
Reporter | ||
Updated•14 years ago
|
See Also: → https://bugs.webkit.org/show_bug.cgi?id=35607
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
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: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 3•14 years ago
|
||
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.
Reporter | ||
Comment 5•14 years ago
|
||
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
Comment 6•14 years ago
|
||
Where are you expecting MSVC to declare uintptr_t? Mine has it in stddef.h but nptypes.h doesn't include that.
Reporter | ||
Comment 7•14 years ago
|
||
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?
Comment 9•14 years ago
|
||
Ah, so windef.h includes winnt.h includes ctype.h that includes crtdefs.h that includes vadefs.h that defines uintptr_t.
Assignee | ||
Comment 10•14 years ago
|
||
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 11•14 years ago
|
||
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 12•14 years ago
|
||
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+
Assignee | ||
Comment 13•14 years ago
|
||
pushed to mozilla-1.9.2 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5821b3eb4692
status1.9.2:
--- → .9-fixed
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•