User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:220.127.116.11) Gecko/2009021910 Firefox/3.0.7 (.NET CLR 3.5.30729) Build Identifier: self-built NSPR for use in Evolution In pr/src/md/windows/ntsec.c, in the function _PR_NT_InitSids(), when built with gcc 4.3.3 with optimization, the infoBuffer happens to be aligned on a two-char boundary. Apparently GetTokenInformation() wants it to be aligned on a word, or perhaps more generally (thinking about x64), pointer size boundary. If not, it returns failure, and the code asserts or crashes. Try the test program (which includes _PR_NT_InitSids() as such with just a debugging printf() added) pointed to above with gcc 4.3.3 with -O2 and -O0. A simple fix is to change UCHAR infoBuffer; to PVOID infoBuffer[1024/sizeof(PVOID)]; Reproducible: Always
Thanks for the bug report and the suggested fix. Looking at the MSDN page for GetTokenInformation: http://msdn.microsoft.com/en-us/library/aa446671(VS.85).aspx I see the TOKEN_OWNER and TOKEN_PRIMARY_GROUP structures both start with a pointer (of type PSID), so you're right that infoBuffer should be aligned on a pointer size boundary. An alternative fix is to not use a fixed-sized stack buffer. We can call GetTokenInformation with a null pointer to get the necessary buffer size, call malloc to allocate a buffer of that size (malloc takes care of alignment), and call GetTokenInformation again with that buffer. This is what the two examples on the GetTokenInformation MSDN page do.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Created attachment 376160 [details] [diff] [review] Tor Lillqvist's patch (checked in) Tor, could you test this patch with gcc? It's the same as your patch except that I changed PVOID to PSID and added a comment. Thanks!
Attachment #376160 - Flags: review?(nelson)
I don't have a test environment ready right now, but I am sure that will work, too.
How does GetTokenInformation fail? Given that the x86 family of CPUs (including both 32 and 64 bit) have no alignment requirements), Is GetTokenInformation testing the alignment and returning some error?
Yes, I guess GetTokenInformation is testing the alignment. If I pass an unaligned buffer to GetTokenInformation, it returns FALSE with the error 998 (ERROR_NOACCESS, "Invalid access to memory location").
Comment on attachment 376160 [details] [diff] [review] Tor Lillqvist's patch (checked in) Is sizeof(PSID) always a power of 2?
Comment on attachment 376160 [details] [diff] [review] Tor Lillqvist's patch (checked in) It appears that the declaration of PSID is: typedef PVOID PSID; So, PROVIDED that 4-byte alignment is good enough on Win32, r+. Otherwise, it will be necessary to use some 64-bit type.
Attachment #376160 - Flags: review?(nelson) → review+
Created attachment 376275 [details] [diff] [review] Avoid dividing by sizeof(PSID) Nelson, this is a variant of Tor Lillqvist's patch that I considered. Let me know if you prefer this patch. It avoids the issue of whether sizeof(PSID) divides 1024 evenly. infoBuffer is cast to pointers to TOKEN_OWNER and TOKEN_PRIMARY_GROUP, both of which contain just a PSID member: http://msdn.microsoft.com/en-us/library/aa379628(VS.85).aspx http://msdn.microsoft.com/en-us/library/aa379629(VS.85).aspx Presumably that PSID member points to data stored by GetTokenInformation in the rest of infoBuffer, and GetTokenInformation should take care of aligning those data.
Attachment #376275 - Flags: review?(nelson)
Comment on attachment 376275 [details] [diff] [review] Avoid dividing by sizeof(PSID) This seems as good as the previous patch. I wonder. Does this assure alignment? Does the gcc compiler align stack variables even on CPUs that don't need it? (If so, then why does this bug exist?) Might it be necessary to write some code to do explicit pointer arithmetic? e.g. PRUint8 buffer[1024+8]; ptrdiff_t bufaddr = (ptrdiff_t)&buffer; PSID * psids = (PSID *)(bufaddr - (bufaddr & 0x7));
Attachment #376275 - Flags: review?(nelson) → review+
Attachment #376160 - Attachment description: Tor Lillqvist's patch → Tor Lillqvist's patch (checked in)
Comment on attachment 376160 [details] [diff] [review] Tor Lillqvist's patch (checked in) An array of PSIDs is aligned on PSID boundary. So the only issue is whether we need stronger alignment (on 8-byte boundary). If so, we can use an array of doubles, which the PLArena code assumes to represent the strongest alignment requirement: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/lib/ds/plarena.c&rev=3.16&mark=63#58 But I think alignment on PSID boundary is enough. I checked in the patch on the NSPR trunk (NSPR 4.8). Checking in ntsec.c; /cvsroot/mozilla/nsprpub/pr/src/md/windows/ntsec.c,v <-- ntsec.c new revision: 3.9; previous revision: 3.8 done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8
(In reply to comment #3) > I don't have a test environment ready right now, but I am sure that will work, > too. Tor, since I have reproduced the GetTokenInformation() failure, you don't need to test the patch any more. Thanks!
You need to log in before you can comment on or make changes to this bug.