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.