I believe this one causes problems -- please do not commit yet
Joris Huizer joris_huizer@yahoo.com wrote:
--------------------------------- Need Mail bonding? Go to the Yahoo! Mail Q&A for great tips from Yahoo! Answers users.>From 8686a200bd1f1cbd934d761013c487a9ddc195c9 Mon Sep 17 00:00:00 2001 From: Joris Huizer Date: Wed, 7 Feb 2007 15:19:25 +0100 Subject: [PATCH] winegcc sign-compare fixes To: wine-patches
--- tools/winegcc/utils.c | 13 +++++++------ tools/winegcc/winegcc.c | 11 ++++++----- 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/tools/winegcc/utils.c b/tools/winegcc/utils.c index 1af91e9..c144077 100644 --- a/tools/winegcc/utils.c +++ b/tools/winegcc/utils.c @@ -120,14 +120,14 @@ void strarray_add(strarray* arr, const c
void strarray_del(strarray* arr, int i) { - if (i < 0 || i >= arr->size) error("Invalid index i=%d", i); + if (i < 0 || (unsigned int)i >= arr->size) error("Invalid index i=%d", i); memmove(&arr->base[i], &arr->base[i + 1], (arr->size - i - 1) * sizeof(arr->base[0])); arr->size--; }
void strarray_addall(strarray* arr, const strarray* from) { - int i; + unsigned int i;
for (i = 0; i < from->size; i++) strarray_add(arr, from->base[i]); @@ -136,7 +136,7 @@ void strarray_addall(strarray* arr, cons strarray* strarray_dup(const strarray* arr) { strarray* dup = strarray_alloc(); - int i; + unsigned int i;
for (i = 0; i < arr->size; i++) strarray_add(dup, arr->base[i]); @@ -160,7 +160,7 @@ strarray* strarray_fromstring(const char char* strarray_tostring(const strarray* arr, const char* sep) { char *str, *newstr; - int i; + unsigned int i;
str = strmake("%s", arr->base[0]); for (i = 1; i < arr->size; i++) @@ -277,7 +277,7 @@ static file_type guess_lib_type(const ch
file_type get_lib_type(strarray* path, const char* library, char** file) { - int i; + unsigned int i;
for (i = 0; i < path->size; i++) { @@ -289,7 +289,8 @@ file_type get_lib_type(strarray* path, c
void spawn(const strarray* prefix, const strarray* args, int ignore_errors) { - int i, status; + unsigned int i; + int status; strarray* arr = strarray_dup(args); const char** argv; char* prog = 0; diff --git a/tools/winegcc/winegcc.c b/tools/winegcc/winegcc.c index f1aa87e..f1a7bb0 100644 --- a/tools/winegcc/winegcc.c +++ b/tools/winegcc/winegcc.c @@ -170,7 +170,7 @@ struct options
static void clean_temp_files(void) { - int i; + unsigned int i;
if (keep_generated) return;
@@ -239,7 +239,8 @@ static const strarray* get_translator(en static void compile(struct options* opts, const char* lang) { strarray* comp_args = strarray_alloc(); - int j, gcc_defs = 0; + unsigned int j; + int gcc_defs = 0;
switch(opts->processor) { @@ -434,7 +435,7 @@ static void build(struct options* opts) const char *output_name, *spec_file, *lang; const char* winebuild = getenv("WINEBUILD"); int generate_app_loader = 1; - int j; + unsigned int j;
/* NOTE: for the files array we'll use the following convention: * -axxx: xxx is an archive (.a) @@ -719,7 +720,7 @@ static int is_linker_arg(const char* arg "-static", "-static-libgcc", "-shared", "-shared-libgcc", "-symbolic", "-framework" }; - int j; + unsigned int j;
switch (arg[1]) { @@ -769,7 +770,7 @@ static int is_mingw_arg(const char* arg) { "-mno-cygwin", "-mwindows", "-mconsole", "-mthreads", "-municode" }; - int j; + unsigned int j;
for (j = 0; j < sizeof(mingw_switches)/sizeof(mingw_switches[0]); j++) if (strcmp(mingw_switches[j], arg) == 0) return 1;
Joris Huizer joris_huizer@yahoo.com wrote: I believe this one causes problems -- please do not commit yet
Sorry for the spamming - the patch should be alright
--------------------------------- Everyone is raving about the all-new Yahoo! Mail beta.
On 2/7/07, Joris Huizer joris_huizer@yahoo.com wrote:
I believe this one causes problems -- please do not commit yet
Joris Huizer joris_huizer@yahoo.com wrote:
Need Mail bonding? Go to the Yahoo! Mail Q&A for great tips from Yahoo! Answers users.>From 8686a200bd1f1cbd934d761013c487a9ddc195c9 Mon Sep 17 00:00:00 2001 From: Joris Huizer Date: Wed, 7 Feb 2007 15:19:25 +0100 Subject: [PATCH] winegcc sign-compare fixes To: wine-patches
tools/winegcc/utils.c | 13 +++++++------ tools/winegcc/winegcc.c | 11 ++++++----- 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/tools/winegcc/utils.c b/tools/winegcc/utils.c index 1af91e9..c144077 100644 --- a/tools/winegcc/utils.c +++ b/tools/winegcc/utils.c @@ -120,14 +120,14 @@ void strarray_add(strarray* arr, const c
void strarray_del(strarray* arr, int i) {
- if (i < 0 || i >= arr->size) error("Invalid index i=%d", i);
- if (i < 0 || (unsigned int)i >= arr->size) error("Invalid index i=%d", i);
The unsigned int comparison here makes the negative check superfluous. Perhaps the argument should even be changed to unsigned as well, to match the range of arr->size (if the element is one byte).
memmove(&arr->base[i], &arr->base[i + 1], (arr->size - i - 1) * sizeof(arr->base[0])); arr->size--; }
If the element size could be greater than one byte, the multiplication here should be checked for overflow (actually, this is more necessary when the array is created or expanded).
--- "richardvoigt@gmail.com" richardvoigt@gmail.com wrote:
On 2/7/07, Joris Huizer joris_huizer@yahoo.com wrote:
I believe this one causes problems -- please do
not commit yet
Joris Huizer joris_huizer@yahoo.com wrote:
Need Mail bonding? Go to the Yahoo! Mail Q&A for great tips from
Yahoo! Answers users.>From
8686a200bd1f1cbd934d761013c487a9ddc195c9 Mon Sep
17
00:00:00 2001 From: Joris Huizer Date: Wed, 7 Feb 2007 15:19:25 +0100 Subject: [PATCH] winegcc sign-compare fixes To: wine-patches
tools/winegcc/utils.c | 13 +++++++------ tools/winegcc/winegcc.c | 11 ++++++----- 2 files changed, 13 insertions(+), 11
deletions(-)
diff --git a/tools/winegcc/utils.c
b/tools/winegcc/utils.c
index 1af91e9..c144077 100644 --- a/tools/winegcc/utils.c +++ b/tools/winegcc/utils.c @@ -120,14 +120,14 @@ void strarray_add(strarray*
arr, const c
void strarray_del(strarray* arr, int i) {
- if (i < 0 || i >= arr->size) error("Invalid
index i=%d", i);
- if (i < 0 || (unsigned int)i >= arr->size)
error("Invalid index i=%d", i);
The unsigned int comparison here makes the negative check superfluous. Perhaps the argument should even be changed to unsigned as well, to match the range of arr->size (if the element is one byte).
Alright, that makes sence, will do
memmove(&arr->base[i], &arr->base[i + 1],
(arr->size - i - 1) *
sizeof(arr->base[0])); arr->size--; }
If the element size could be greater than one byte, the multiplication here should be checked for overflow (actually, this is more necessary when the array is created or expanded).
Well, the base field in the structure is defined as "const char** base" so sizeof(base[0]) == sizeof(const char *), should be 4 or so. How do you mean overflow in this case? sorry for asking dumb questions...
____________________________________________________________________________________ Finding fabulous fares is fun. Let Yahoo! FareChase search your favorite travel sites to find flight and hotel bargains. http://farechase.yahoo.com/promo-generic-14795097
memmove(&arr->base[i], &arr->base[i + 1],
(arr->size - i - 1) *
sizeof(arr->base[0])); arr->size--; }
If the element size could be greater than one byte, the multiplication here should be checked for overflow (actually, this is more necessary when the array is created or expanded).
Well, the base field in the structure is defined as "const char** base" so sizeof(base[0]) == sizeof(const char *), should be 4 or so. How do you mean overflow in this case? sorry for asking dumb questions...
One desires that the argument is the size of memory which holds numelems = (arr->size - i - 1) elements of each = sizeof (arr->base[0]) bytes each. As you say each = 4 bytes on 32-bit systems.
Then the value is numelems * each = numelems * 4 = numelems << 2. Thus if either of the two most significant bits in numelems are set, they will be lost as a result of the shift (multiplication). In this case the correct value is larger than addressable virtual memory, which should be illegal. Actually though, the limit should be placed on arr->size in the allocation function.
The essential problem here is that this can be used to remove sanity checking in the case where the inputs are supplied by a client with lesser permissions. Imagine, for example, a kernel function being invoked via an ioctl... though the problem is equally valid for any network-facing service.
Say a client is allowed to create and access buffer for exchange with a device (USB printer or the like), and that the client can allocate the buffer by providing the number of buffer elements, and the element size is fixed and non-unity, we'll use 4 in this example. If the privileged service does something like the following:
handle createbuffer(int buflen) { if (buflen <= 0) return NULL int bytecount = 4 * buflen; if (bytecount > quota) return NULL; int* p = malloc(bytecount); if (!p) return NULL; return createbufferhandle(buflen, p); }
and lets the client read from the buffer: int readbufferatindex(handle buffer, int index) { int size = buffersizefromhandle(buffer); if (index < 0 || index >= size) return 0; return bufferptrfromhandle(buffer)[index]; }
Seems safe enough, right? Each client is allowed only a limited buffer, maybe elsewhere the number of buffers is limited, so the client can't execute a DOS attack. malloc guarantees that the buffer doesn't overlap any other buffer, and the bounds checking in readbufferatindex prevents clients from reading outside their buffer.
Until I do this: buffer = createbuffer(0x40000010);
Uh-oh. I passed the quota check, because bytecount was calculated at 64 bytes. malloc easily allocated 64 bytes not overlapping with any other user. But the stored buffersize is set so large that the bounds check is worthless; I can read any memory I want.
-
Joris Huizer -
richardvoigt＠gmail.com