Re: [PATCH] shell32: use flexible arrays to avoid fortify failures
Mike Frysinger <vapier(a)gentoo.org> writes:
Newer versions of gcc/glibc with fortify checks enabled will complain about the handling of the network's szNames field. Currently it is always defined with a length of one which means writing more then a single byte will trigger: In function 'strcpy', inlined from '_ILCreateEntireNetwork' at dlls/shell32/pidl.c:1762:15: warning: call to __builtin___strcpy_chk will always overflow destination buffer and then at runtime, we hit an abort().
Since this field is really serving as the header to an arbitrary buffer, using a flexible array instead should solve the issue.
What you want is ANYSIZE_ARRAY, which already exists. -- Alexandre Julliard julliard(a)winehq.org
On Sunday, September 19, 2010 08:58:42 Alexandre Julliard wrote:
Mike Frysinger writes:
Newer versions of gcc/glibc with fortify checks enabled will complain about the handling of the network's szNames field. Currently it is always defined with a length of one which means writing more then a
single byte will trigger: In function 'strcpy', inlined from '_ILCreateEntireNetwork' at dlls/shell32/pidl.c:1762:15: warning: call to __builtin___strcpy_chk will always overflow destination buffer
and then at runtime, we hit an abort().
Since this field is really serving as the header to an arbitrary buffer, using a flexible array instead should solve the issue.
What you want is ANYSIZE_ARRAY, which already exists.
i dont see how that would help. the code is currently: typedef struct ... { ... CHAR foo[1]; } ...; it needs to be "foo[]". ANYSIZE_ARRAY is defined as 1, so once the preprocessor is done, we're right back where we started. i cant change the value of ANYSIZE_ARRAY to nothing because some code uses this define in multiplication to calculate the size of objects. -mike
On 9/19/10 11:53 AM, Mike Frysinger wrote:
On Sunday, September 19, 2010 08:58:42 Alexandre Julliard wrote:
Mike Frysinger writes:
Newer versions of gcc/glibc with fortify checks enabled will complain about the handling of the network's szNames field. Currently it is always defined with a length of one which means writing more then a
single byte will trigger: In function 'strcpy', inlined from '_ILCreateEntireNetwork' at dlls/shell32/pidl.c:1762:15: warning: call to __builtin___strcpy_chk will always overflow destination buffer
and then at runtime, we hit an abort().
Since this field is really serving as the header to an arbitrary buffer, using a flexible array instead should solve the issue. What you want is ANYSIZE_ARRAY, which already exists. i dont see how that would help. the code is currently: typedef struct ... { ... CHAR foo[1]; } ...;
it needs to be "foo[]". Just a question to satisfy my curiousity:
Is this supported in C89 code? If not, then the char[1] has to remain because the code level is C89. http://wiki.winehq.org/SubmittingPatches#head-7a578af49f1d1ab7f36f4b1f98b754... States to avoid C++, C99, and non-portable C constructs. James McKenzie Just a Humble Reader, not a true developer
Mike Frysinger <vapier(a)gentoo.org> writes:
i dont see how that would help. the code is currently: typedef struct ... { ... CHAR foo[1]; } ...;
it needs to be "foo[]". ANYSIZE_ARRAY is defined as 1, so once the preprocessor is done, we're right back where we started. i cant change the value of ANYSIZE_ARRAY to nothing because some code uses this define in multiplication to calculate the size of objects.
True, you don't want to change the public one, but you can define a shell32-specific version of it. At least then it looks like a normal array declaration. -- Alexandre Julliard julliard(a)winehq.org
On Sunday, September 19, 2010 17:17:53 Alexandre Julliard wrote:
Mike Frysinger writes:
i dont see how that would help. the code is currently: typedef struct ... {
... CHAR foo[1];
} ...;
it needs to be "foo[]". ANYSIZE_ARRAY is defined as 1, so once the preprocessor is done, we're right back where we started. i cant change the value of ANYSIZE_ARRAY to nothing because some code uses this define in multiplication to calculate the size of objects.
True, you don't want to change the public one, but you can define a shell32-specific version of it. At least then it looks like a normal array declaration.
what do you mean by "public" ? the header i'm changing (dlls/shell32/pidl.h) isnt exported during `make install` that i can see. so only wine itself will be reading this header, and i think all consumers of it internally should be getting "foo[]". -mike
Mike Frysinger <vapier(a)gentoo.org> writes:
On Sunday, September 19, 2010 17:17:53 Alexandre Julliard wrote:
Mike Frysinger writes:
i dont see how that would help. the code is currently: typedef struct ... {
... CHAR foo[1];
} ...;
it needs to be "foo[]". ANYSIZE_ARRAY is defined as 1, so once the preprocessor is done, we're right back where we started. i cant change the value of ANYSIZE_ARRAY to nothing because some code uses this define in multiplication to calculate the size of objects.
True, you don't want to change the public one, but you can define a shell32-specific version of it. At least then it looks like a normal array declaration.
what do you mean by "public" ? the header i'm changing (dlls/shell32/pidl.h) isnt exported during `make install` that i can see. so only wine itself will be reading this header, and i think all consumers of it internally should be getting "foo[]".
Yes. My initial thought was that you could improve the global ANYSIZE_ARRAY, but as you pointed out that's not a good idea. What you should do then is define SHELL32_ANYSIZE_ARRAY in pidl.h, that you use only in pidl.h, and not touch the global headers at all. -- Alexandre Julliard julliard(a)winehq.org
On Monday, September 20, 2010 06:49:20 Alexandre Julliard wrote:
Mike Frysinger writes:
On Sunday, September 19, 2010 17:17:53 Alexandre Julliard wrote:
Mike Frysinger writes:
i dont see how that would help. the code is currently: typedef struct ... {
... CHAR foo[1];
} ...;
it needs to be "foo[]". ANYSIZE_ARRAY is defined as 1, so once the preprocessor is done, we're right back where we started. i cant change the value of ANYSIZE_ARRAY to nothing because some code uses this define in multiplication to calculate the size of objects.
True, you don't want to change the public one, but you can define a shell32-specific version of it. At least then it looks like a normal array declaration.
what do you mean by "public" ? the header i'm changing (dlls/shell32/pidl.h) isnt exported during `make install` that i can see. so only wine itself will be reading this header, and i think all consumers of it internally should be getting "foo[]".
Yes. My initial thought was that you could improve the global ANYSIZE_ARRAY, but as you pointed out that's not a good idea. What you should do then is define SHELL32_ANYSIZE_ARRAY in pidl.h, that you use only in pidl.h, and not touch the global headers at all.
well, i dont think this issue is limited to shell32. it's just the only one to hit it atm. what about my other patch i posted ? http://www.winehq.org/pipermail/wine-patches/2010-September/093377.html -mike
On 20 September 2010 17:51, Mike Frysinger <vapier(a)gentoo.org> wrote:
well, i dont think this issue is limited to shell32. it's just the only one to hit it atm. what about my other patch i posted ? http://www.winehq.org/pipermail/wine-patches/2010-September/093377.html
How does fortify work? See http://blogs.msdn.com/b/oldnewthing/archive/2004/08/26/220873.aspx for information on how to allocate these structures. Specifically: PTOKEN_GROUPS TokenGroups = malloc(FIELD_OFFSET(TOKEN_GROUPS, Groups[NumberOfGroups])); The article explains that: PTOKEN_GROUPS TokenGroups = malloc(sizeof(TOKEN_GROUPS) + NumberOfGroups * sizeof(SID_AND_ATTRIBUTES)); crashes on 64-bit platforms with STATUS_DATATYPE_MISALIGNMENT due to the data being placed on a 4-byte, not 8-byte, boundary. Is the shell32 code running into something similar -- that is, are the calculations for the allocated memory blocks using these ANYSIZE_ARRAY structures wrong? - Reece
On Tuesday, September 21, 2010 03:34:33 Reece Dunn wrote:
On 20 September 2010 17:51, Mike Frysinger <vapier(a)gentoo.org> wrote:
well, i dont think this issue is limited to shell32. it's just the only one to hit it atm. what about my other patch i posted ? http://www.winehq.org/pipermail/wine-patches/2010-September/093377.html
How does fortify work?
fortify is only adding security/sanity checks to functions. so if you do: char f[1]; strcpy(f, "1234"); the C library, with help from the compiler, will then perform constant checks on these things. since 5 bytes is more than the storage of "f" can hold, you get a build time warning. and then at runtime, if this code is attempted to be executed, it will abort() before the storage is allowed to overflow. the problem with the wine code is that it declares a buffer as 1 byte long even though in reality it is the start of a flexible string. newer C specs account for this behavior by introducing the "[]" syntax. the C library will not perform length checks on these strings since it has no idea what its limits are at build time. this isnt really an allocation issue as i think your line of logic is going. -mike
On 21 September 2010 08:58, Mike Frysinger <vapier(a)gentoo.org> wrote:
On Tuesday, September 21, 2010 03:34:33 Reece Dunn wrote:
On 20 September 2010 17:51, Mike Frysinger <vapier(a)gentoo.org> wrote:
well, i dont think this issue is limited to shell32. it's just the only one to hit it atm. what about my other patch i posted ? http://www.winehq.org/pipermail/wine-patches/2010-September/093377.html
How does fortify work?
fortify is only adding security/sanity checks to functions. so if you do: char f[1]; strcpy(f, "1234"); the C library, with help from the compiler, will then perform constant checks on these things. since 5 bytes is more than the storage of "f" can hold, you get a build time warning. and then at runtime, if this code is attempted to be executed, it will abort() before the storage is allowed to overflow.
the problem with the wine code is that it declares a buffer as 1 byte long even though in reality it is the start of a flexible string. newer C specs account for this behavior by introducing the "[]" syntax. the C library will not perform length checks on these strings since it has no idea what its limits are at build time.
Ah, I see. You could always do something like: strcpy((char *)pidl->anysize, "1234"); Which would force the compiler to use the char * version instead of the char [n] version of the strcpy function in this example. This would then work in any compiler without special casing for compilers that have fortify -- especially when public structures get impacted. - Reece
On Tuesday, September 21, 2010 04:21:28 Reece Dunn wrote:
On 21 September 2010 08:58, Mike Frysinger wrote:
fortify is only adding security/sanity checks to functions. so if you do: char f[1]; strcpy(f, "1234"); the C library, with help from the compiler, will then perform constant checks on these things. since 5 bytes is more than the storage of "f" can hold, you get a build time warning. and then at runtime, if this code is attempted to be executed, it will abort() before the storage is allowed to overflow.
the problem with the wine code is that it declares a buffer as 1 byte long even though in reality it is the start of a flexible string. newer C specs account for this behavior by introducing the "[]" syntax. the C library will not perform length checks on these strings since it has no idea what its limits are at build time.
Ah, I see.
You could always do something like:
strcpy((char *)pidl->anysize, "1234");
Which would force the compiler to use the char * version instead of the char [n] version of the strcpy function in this example.
This would then work in any compiler without special casing for compilers that have fortify -- especially when public structures get impacted.
no, that wouldnt help. the compiler is too smart and is still able to propagate the constant storage information to the checking code. someone suggested that in a past thread on this topic. -mike
On Tue, Sep 21, 2010 at 08:34:33AM +0100, Reece Dunn wrote:
On 20 September 2010 17:51, Mike Frysinger <vapier(a)gentoo.org> wrote:
well, i dont think this issue is limited to shell32. it's just the only one to hit it atm. what about my other patch i posted ? http://www.winehq.org/pipermail/wine-patches/2010-September/093377.html
How does fortify work?
It detects the structure size.
See http://blogs.msdn.com/b/oldnewthing/archive/2004/08/26/220873.aspx for information on how to allocate these structures. Specifically:
PTOKEN_GROUPS TokenGroups = malloc(FIELD_OFFSET(TOKEN_GROUPS, Groups[NumberOfGroups]));
The article explains that:
PTOKEN_GROUPS TokenGroups = malloc(sizeof(TOKEN_GROUPS) + NumberOfGroups * sizeof(SID_AND_ATTRIBUTES));
crashes on 64-bit platforms with STATUS_DATATYPE_MISALIGNMENT due to the data being placed on a 4-byte, not 8-byte, boundary.
Is the shell32 code running into something similar -- that is, are the calculations for the allocated memory blocks using these ANYSIZE_ARRAY structures wrong?
No, its just that the structure is embedded in another structure and gcc 4.5 only looks at the size of the inner structure for these variable array, and so does not see it is large enough allocated. (It is kinda in a gray area, but I am tending towards gcc a bit wrong.) Ciao, Marcus
On 09/21/2010 03:43 AM, Marcus Meissner wrote:
No, its just that the structure is embedded in another structure and gcc 4.5 only looks at the size of the inner structure for these variable array, and so does not see it is large enough allocated.
(It is kinda in a gray area, but I am tending towards gcc a bit wrong.)
I tend to agree. buffer[1] instead of buffer[] is part of many structures for a good reason. It accounts for terminating \0 in strings. When you allocate such a struct all you have to do is malloc(sizeof(struct) + strlen(string)). With "buffer[]" declaration one have to add extra byte to size calculations. Besides all this doesn't really help you much with compile time checking. Compiler either wrongly complains about potential buffer overrun or doesn't check the size of the buffer at all. So IMHO disabling this check completely or for such structures is a better way to go. Vitaliy.
participants (6)
-
Alexandre Julliard -
James McKenzie -
Marcus Meissner -
Mike Frysinger -
Reece Dunn -
Vitaliy Margolen