The buffer passed to GetTokenInformation apparently needs to be word-aligned

RESOLVED FIXED in 4.8

Status

NSPR
NSPR
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Tor Lillqvist, Assigned: Wan-Teh Chang)

Tracking

4.7.3
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

1.12 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
1.09 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.7) 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[1024];

to
PVOID infoBuffer[1024/sizeof(PVOID)];


Reproducible: Always
(Reporter)

Updated

9 years ago
Version: other → 4.7.3
(Assignee)

Comment 1

9 years ago
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
(Assignee)

Comment 2

9 years ago
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)
(Reporter)

Comment 3

9 years ago
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?
(Assignee)

Comment 5

9 years ago
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+
(Assignee)

Comment 8

9 years ago
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[7];
   PSID *    psids   = (PSID *)(bufaddr - (bufaddr & 0x7));
Attachment #376275 - Flags: review?(nelson) → review+
(Assignee)

Updated

9 years ago
Attachment #376160 - Attachment description: Tor Lillqvist's patch → Tor Lillqvist's patch (checked in)
(Assignee)

Comment 10

9 years ago
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
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8
(Assignee)

Comment 11

9 years ago
(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.