RtlAppendUnicodeStringToString

[O_TEXT]

Why the last line?



NTSTATUS
STDCALL
RtlAppendUnicodeStringToString(
IN OUT PUNICODE_STRING Destination,
IN PUNICODE_STRING Source)
{

if ((Source->Length + Destination->Length) >=
Destination->MaximumLength)
return STATUS_BUFFER_TOO_SMALL;

memcpy((PVOID)Destination->Buffer + Destination->Length,
Source->Buffer, Source->Length);
Destination->Length += Source->Length;
if( Destination->MaximumLength > Destination->Length )
Destination->Buffer[Destination->Length / sizeof(WCHAR)] = 0;

return STATUS_SUCCESS;
}




drivers/filesystems/fastfat/fcb.c (extraits):

PathNameLength = directoryFCB->PathNameU.Length +
max(DirContext->LongNameU.Length,
DirContext->ShortNameU.Length);
if (!vfatFCBIsRoot (directoryFCB))
{
PathNameLength += sizeof(WCHAR);
}

if (PathNameLength > LONGNAME_MAX_LENGTH * sizeof(WCHAR))
{
return STATUS_OBJECT_NAME_INVALID;
}
PathNameBuffer = ExAllocatePoolWithTag(NonPagedPool,
PathNameLength + sizeof(WCHAR), TAG_FCB);
if (!PathNameBuffer)
{
return STATUS_INSUFFICIENT_RESOURCES;
}
NameU.Buffer = PathNameBuffer;
NameU.Length = 0;
NameU.MaximumLength = PathNameLength;
RtlCopyUnicodeString(&NameU, &directoryFCB->PathNameU);
if (!vfatFCBIsRoot (directoryFCB))
{
RtlAppendUnicodeToString(&NameU, L"\\");
}
hash = vfatNameHash(0, &NameU);
if (DirContext->LongNameU.Length > 0)
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->LongNameU);
}
else
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->ShortNameU);
}
NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

 

[O_TEXT]

O_TEXT a écrit :

Why the last line?


trunk/reactos/drivers/fs/vfat

NTSTATUS
STDCALL
RtlAppendUnicodeStringToString(
IN OUT PUNICODE_STRING Destination,
IN PUNICODE_STRING Source)
{

if ((Source->Length + Destination->Length) >=
Destination->MaximumLength)
return STATUS_BUFFER_TOO_SMALL;

memcpy((PVOID)Destination->Buffer + Destination->Length,
Source->Buffer, Source->Length);
Destination->Length += Source->Length;
if( Destination->MaximumLength > Destination->Length )
Destination->Buffer[Destination->Length / sizeof(WCHAR)] = 0;

return STATUS_SUCCESS;
}

drivers/filesystems/fastfat/fcb.c (extraits):

PathNameLength = directoryFCB->PathNameU.Length +
max(DirContext->LongNameU.Length,
DirContext->ShortNameU.Length);
if (!vfatFCBIsRoot (directoryFCB))
{
PathNameLength += sizeof(WCHAR);
}

if (PathNameLength > LONGNAME_MAX_LENGTH * sizeof(WCHAR))
{
return STATUS_OBJECT_NAME_INVALID;
}
PathNameBuffer = ExAllocatePoolWithTag(NonPagedPool,
PathNameLength + sizeof(WCHAR), TAG_FCB);
if (!PathNameBuffer)
{
return STATUS_INSUFFICIENT_RESOURCES;
}
NameU.Buffer = PathNameBuffer;
NameU.Length = 0;
NameU.MaximumLength = PathNameLength;
RtlCopyUnicodeString(&NameU, &directoryFCB->PathNameU);
if (!vfatFCBIsRoot (directoryFCB))
{
RtlAppendUnicodeToString(&NameU, L"\\");
}
hash = vfatNameHash(0, &NameU);
if (DirContext->LongNameU.Length > 0)
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->LongNameU);
}
else
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->ShortNameU);
}
NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

I am wondering if this is not an overflow, because:

Allocation is computed as:
directoryFCB->PathNameU.Length +
max(DirContext->LongNameU.Length,DirContext->ShortNameU.Length)
+ sizeof(WCHAR)

And copied characters are:
directoryFCB->PathNameU
L"\\"
DirContext->LongNameU or DirContext->ShortNameU
0


I understand that as n characters allocated, but n+1 written...

 

[Giacomo Degli Esposti]

I am wondering if this is not an overflow, because:

Allocation is computed as:
    directoryFCB->PathNameU.Length +
max(DirContext->LongNameU.Length,DirContext->ShortNameU.Length)
          + sizeof(WCHAR)

And copied characters are:
    directoryFCB->PathNameU
    L"\\"
DirContext->LongNameU or DirContext->ShortNameU
    0

I understand that as n characters allocated, but n+1 written

It looks like in allocation you have +sizeof(WCHAR), giving n+1 to
allocation too.

ciao
Giacomo

 

[Jens Thoms Toerring]

The whole code is hard to understand since you're not telling
what exactly the structures are you're using here. What, for
example, is a 'PUNICODE_STRING'? It seems to be a pointer to
a structure having at least three members, 'Buffer', 'Length'
and 'MaximumLength'. But what exactly these members are and
how they are supposed to be used can only be guessed at.

And then there are lots of non-standard C functions and
constructs, so also there one can only guess at best.

trunk/reactos/drivers/fs/vfat

What's 'STDCALL'? That's not C.

What are 'IN OUT' and 'IN' meant to be?

Too me it looks as if the comparison operator here should
be '>' and not '>='. But, of course, I might be wrong since
it's not 100% clear what the members of the structure are
supposed to stand for.

If '(PVOID)' is what it looks like, i.e. a cast to a void
pointer, then this is wrong since arithmetic on void pointers
isn't allowed. This only may work if the compiler has some ex-
tension that makes it "right" (like e.g. gcc has). The correct
cast would be to 'char *' (assuming that the 'Length' members
are the lengths of the strings in bytes, which is the only
thing that seems to make sense from context).

Destination->Length += Source->Length;
if( Destination->MaximumLength > Destination->Length )
Destination->Buffer[Destination->Length / sizeof(WCHAR)] = 0;

I have no idea why this isn't done unconditionally.

drivers/filesystems/fastfat/fcb.c (extraits):

What does this function do? Is it possible that it changes
the 'NameU' structure? Then all bets would be off.

if (DirContext->LongNameU.Length > 0)
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->LongNameU);
}
else
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->ShortNameU);
}
NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

That probably is needed here because it's not done in the
RtlAppendUnicodeStringToString() function. Why it's not done
there is beyond me.

I am wondering if this is not an overflow, because:

Allocation is computed as:
directoryFCB->PathNameU.Length +
max(DirContext->LongNameU.Length,DirContext->ShortNameU.Length)
+ sizeof(WCHAR)

And copied characters are:
directoryFCB->PathNameU
L"\\"
DirContext->LongNameU or DirContext->ShortNameU
0

I understand that as n characters allocated, but n+1 written...

'L"\\"' only gets appended if the '(!vfatFCBIsRoot (directoryFCB))'
conditon is satisfied. And if this condition is satisfied then a
bit above in the code (and before the allocation) 'PathNameLength'
is incremented by the size of a WCHAR. So there don't seem to be
a problem here.

But even if that wouldn't be the case the problem should get
caught in RtlAppendUnicodeToString() since there it's tested
if enough memory is available and the function returns a value
indicating error if there isn't. Alas, the return status is
never tested for in the caller...

Regards, Jens

 

[O_TEXT]

Giacomo Degli Esposti a écrit :

It looks like in allocation you have +sizeof(WCHAR), giving n+1 to
allocation too.

ciao
Giacomo

I mean: n+1 are allocated and n+1+1 written?

 

[O_TEXT]

Jens Thoms Toerring a écrit :

The whole code is hard to understand since you're not telling
what exactly the structures are you're using here. What, for
example, is a 'PUNICODE_STRING'? It seems to be a pointer to
a structure having at least three members, 'Buffer', 'Length'
and 'MaximumLength'. But what exactly these members are and
how they are supposed to be used can only be guessed at.

I agree.

And then there are lots of non-standard C functions and
constructs, so also there one can only guess at best.

Yes. Despite they are not standard, it looks like they compile both with
gcc and msvc compilers.

What's 'STDCALL'? That's not C.

A macro, in the Windows/ReactOS style:

I do not know in the windows context.

For ReactOS it is probably one of:
#define STDCALL __stdcall
#define STDCALL __attribute__ ((stdcall))
#define STDCALL _stdcall

Those are related to the asm convention calls and the way to use the
stack and registers.
This is not an issue as long as there is no embedded asm code and caller
uses the same convention as callee.

What are 'IN OUT' and 'IN' meant to be?

A macro, in the Windows/ReactOS style:
In most ReactOS code, it is defined as:
#define IN
#define OUT

This might mean it is used as comment, or for tools/documentation
extraction.

Too me it looks as if the comparison operator here should
be '>' and not '>='. But, of course, I might be wrong since
it's not 100% clear what the members of the structure are
supposed to stand for.

I agree. I believe this depends on whether we want to be sure a null
character can be appended at end.

If '(PVOID)' is what it looks like, i.e. a cast to a void
pointer,

This code might be not working with some compilers.

But with such code, I do not believe to random issues. I mean if it is
tested and works sometimes, it will work all aways as long as not
recompiled.

then this is wrong since arithmetic on void pointers
isn't allowed. This only may work if the compiler has some ex-
tension that makes it "right" (like e.g. gcc has). The correct
cast would be to 'char *' (assuming that the 'Length' members
are the lengths of the strings in bytes, which is the only
thing that seems to make sense from context).

I agree.

Destination->Length += Source->Length;
if( Destination->MaximumLength > Destination->Length )
Destination->Buffer[Destination->Length / sizeof(WCHAR)] = 0;

I have no idea why this isn't done unconditionally.

In my opinion, this is against buffer overflow.
Might be redundant with first test.

What does this function do? Is it possible that it changes
the 'NameU' structure? Then all bets would be off.

I do not think so:

ULONG vfatNameHash(ULONG hash, PWCHAR name)
{
WCHAR c;
while(c = *name++)
{
c = towlower(c);
hash = (hash + (c << 4) + (c >> 4)) * 11;
}
return hash;
}

if (DirContext->LongNameU.Length > 0)
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->LongNameU);
}
else
{
RtlAppendUnicodeStringToString(&NameU,
&DirContext->ShortNameU);
}
NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

That probably is needed here because it's not done in the
RtlAppendUnicodeStringToString() function. Why it's not done
there is beyond me.

Might be...

'L"\\"' only gets appended if the '(!vfatFCBIsRoot (directoryFCB))'
conditon is satisfied. And if this condition is satisfied then a
bit above in the code (and before the allocation) 'PathNameLength'
is incremented by the size of a WCHAR. So there don't seem to be
a problem here.

Okay for the L"\\", but what's about the 0 appended in the last line?

Is there a buffer overflow risk? Probably not, because of the "be '>'
and not '>='" issue. am I right?

But even if that wouldn't be the case the problem should get
caught in RtlAppendUnicodeToString() since there it's tested
if enough memory is available and the function returns a value
indicating error if there isn't. Alas, the return status is
never tested for in the caller...

You are right.
Testing them is a good thing.

 

[Giacomo Degli Esposti]

Giacomo Degli Esposti a écrit :








I mean: n+1 are allocated and n+1+1 written?

I don't think so. In you original msg you say:

NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

It looks like you are allocating n+1 elements in the buffer and
accessing buffer[n] which is within the allocated space...

ciao
Giacomo

 

[Jens Thoms Toerring]

Jens Thoms Toerring a écrit :

I agree. I believe this depends on whether we want to be sure a null
character can be appended at end.

My impression is that the 'Length' elements store the amount
of memory used for the (wide) string without the trailing 0.
If that's not the case the copying with memcpy() done below
won't work correctly, at least as far as I can see.

And the same seems to hold for the 'MaximumLength' member,
it seems to be the maximum amount of memory available for
the string without the trailing 0. And if I got that right,
then '>' instead of '>=' would be the correct thing to use.

This code might be not working with some compilers.

But with such code, I do not believe to random issues. I mean if it is
tested and works sometimes, it will work all aways as long as not
recompiled.

Question is: why rely on compiler specific extensions when there's
a clean, well-defined way to do it in standard C by using a cast
to char pointer?

And what means "it works as long as not ecompiled"? Or do you mean
"compiled with a different compiler"?

Destination->Length += Source->Length;
if( Destination->MaximumLength > Destination->Length )
Destination->Buffer[Destination->Length / sizeof(WCHAR)] = 0;

I have no idea why this isn't done unconditionally.

In my opinion, this is against buffer overflow.
Might be redundant with first test.

The trailing 0 character seems to be needed in some contexts (e.g.
the vfatNameHash() function used below would have a good chance of
iterating past the end of the text unless there's a trailing 0. It
is ok without the trailing 0 for all functions that use the 'Length'
member to determine the length of the string, but those that expect
a C string (i.e. a 0 terminated string) will need it. So why not
ad ithere?

Ok, now PathNameLength is as long as the directory string part
plus what's neded for the file name (butwithout trailing 0).

If the condition is satisfied now PathNameLength is incremented
for one more WCHAR.

PathNameBuffer now has room for as many WCHARs are in the
path name plus the file name plus one for a trailing 0 and,
if the '!vfatFCBIsRoot()' condition was satisfied, one
extra char.

Here the path name gets copied to NameU.Buffer.

Here a single char gets added

I do not think so:

ULONG vfatNameHash(ULONG hash, PWCHAR name)
{
WCHAR c;
while(c = *name++)
{
c = towlower(c);
hash = (hash + (c << 4) + (c >> 4)) * 11;
}
return hash;
}

That's strange. From the other code it looks as if NameU is
a structure of a type so that a PUNICODE veriable can be used
as a pointer to it. And in this function it's treated as
if would be a (WCHAR) string. But perhaps it works because
the 'Buffer' member is the first member of the structure.
But then this is an extremely ugly hack, it relies on having
a trailing 0 at the end of 'Buffer' (which might be there if
the ExAllocatePoolWithTag() zeros out the memory it returns),
and the call of the vfarNameHash() function would better be
written as

hash = vfatNameHash(0,NameU.Buffer);

And here the file name gets appended.

NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

And there should still be enough room for this trailing 0.
So I can't see any obvious buffer overflow here. There are
other potential issues, but a buffer overflow doesn't seem
to me to be one of them.
Regards, Jens

 

[O_TEXT]

Jens Thoms Toerring a écrit :

My impression is that the 'Length' elements store the amount
of memory used for the (wide) string without the trailing 0.
If that's not the case the copying with memcpy() done below
won't work correctly, at least as far as I can see.

And the same seems to hold for the 'MaximumLength' member,
it seems to be the maximum amount of memory available for
the string without the trailing 0. And if I got that right,
then '>' instead of '>=' would be the correct thing to use.
ok




Question is: why rely on compiler specific extensions when there's
a clean, well-defined way to do it in standard C by using a cast
to char pointer?

And what means "it works as long as not ecompiled"? Or do you mean
"compiled with a different compiler"?

I mean recompiled (compiled again), and yes, with a different compiler.

Destination->Length += Source->Length;
if( Destination->MaximumLength > Destination->Length )
Destination->Buffer[Destination->Length / sizeof(WCHAR)] = 0;
I have no idea why this isn't done unconditionally.

In my opinion, this is against buffer overflow.
Might be redundant with first test.

The trailing 0 character seems to be needed in some contexts (e.g.
the vfatNameHash() function used below would have a good chance of
iterating past the end of the text unless there's a trailing 0. It
is ok without the trailing 0 for all functions that use the 'Length'
member to determine the length of the string, but those that expect
a C string (i.e. a 0 terminated string) will need it. So why not
ad ithere?

I do not know.
The aim of ReactOS is to be windows compatible.
I do not know which convention as been chosen by microsoft.

Ok, now PathNameLength is as long as the directory string part
plus what's neded for the file name (butwithout trailing 0).


If the condition is satisfied now PathNameLength is incremented
for one more WCHAR.

Okay. I was stupid, and did not see this line!
So my initial question has no justification.

PathNameBuffer now has room for as many WCHARs are in the
path name plus the file name plus one for a trailing 0 and,
if the '!vfatFCBIsRoot()' condition was satisfied, one
extra char.
0k


Here the path name gets copied to NameU.Buffer.
okay


Here a single char gets added
yes





That's strange. From the other code it looks as if NameU is
a structure of a type so that a PUNICODE veriable can be used
as a pointer to it. And in this function it's treated as
if would be a (WCHAR) string. But perhaps it works because
the 'Buffer' member is the first member of the structure.
But then this is an extremely ugly hack, it relies on having
a trailing 0 at the end of 'Buffer' (which might be there if
the ExAllocatePoolWithTag() zeros out the memory it returns),
and the call of the vfarNameHash() function would better be
written as

hash = vfatNameHash(0,NameU.Buffer);

I had the code on the web. It was not a clean and recent copy of this
function.

FILE: drivers/fs/vfat/fcb.c revision 37275 contains:

26 : static ULONG vfatNameHash(ULONG hash, PUNICODE_STRING NameU)
27 : {
28 : PWCHAR last;
29 : PWCHAR curr;
30 : register WCHAR c;
31 :
32 : ASSERT(NameU->Buffer[0] != L'.');
33 : curr = NameU->Buffer;
34 : last = NameU->Buffer + NameU->Length / sizeof(WCHAR);
35 :
36 : while(curr < last)
37 : {
38 : c = towlower(*curr++);
39 : hash = (hash + (c << 4) + (c >> 4)) * 11;
40 : }
41 : return hash;
42 : }

And here the file name gets appended.

NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

And there should still be enough room for this trailing 0.
So I can't see any obvious buffer overflow here. There are
other potential issues, but a buffer overflow doesn't seem
to me to be one of them.
Regards, Jens

Okay.

So the conclusion is the code is about clean but contains:
- one unclean construction related to the PVOID
- missing error handling related to STATUS_BUFFER_TOO_SMALL issue.

 

[O_TEXT]

Giacomo Degli Esposti a écrit :

Giacomo Degli Esposti a écrit :

[...]
I am wondering if this is not an overflow, because:
Allocation is computed as:
directoryFCB->PathNameU.Length +
max(DirContext->LongNameU.Length,DirContext->ShortNameU.Length)
+ sizeof(WCHAR)
And copied characters are:
directoryFCB->PathNameU
L"\\"
DirContext->LongNameU or DirContext->ShortNameU
0
I understand that as n characters allocated, but n+1 written
It looks like in allocation you have +sizeof(WCHAR), giving n+1 to
allocation too.
ciao
Giacomo

I mean: n+1 are allocated and n+1+1 written?

I don't think so. In you original msg you say:

NameU.Buffer[NameU.Length / sizeof(WCHAR)] = 0;

It looks like you are allocating n+1 elements in the buffer and
accessing buffer[n] which is within the allocated space...

ciao
Giacomo

At first I missed reading one line of the code.
In fact both of you are right, and there is no issue.
This point has been discussed more extensively with
Jens Thoms Toerring.