Subject:
Re: NULL pointer
From:
Alex Holden ####@####.####
Date:
6 Feb 2001 21:34:28 -0000
Message-Id: <Pine.LNX.4.04.10102062052190.1170-100000@hyperspace.linuxhacker.org>
On Thu, 1 Feb 2001, Gary James wrote:
> This function in microwin/src/nanox/srvfunc.c
> GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
Hi, sorry it's took so long to reply; I've been too busy to work on
Microwindows for the past few weeks.
<snip>
> We are allocating buffer of length "len" then copying into it "thislen"
> bytes. So even if the allocation is happy, the memcpy() could exceed the
> buffer boundary. It looks like the calling function wants a buffer of "len"
> sent back to the client not one of length "thislen".
The first thing you said is definitely right- the function should return
after the GsError().
As to the second part, the logic is indeed incorrect but not in the way
that you think. Thislen is guaranteed to be <= len because the only time
when they are not equal is when the data block has to be fragmented into
multiple packets (in which case len gives the total length of the block
and thislen gives the length of this packet). So allocating an area of
memory the size of the entire block to store the amount of data needed for
the current packet is wasteful and could use up the server's memory if the
block is too large, but it isn't a buffer overrun bug like you suggested.
The correct fix for this would be to change:
if(!(p = malloc(len))) GsError(GR_ERROR_MALLOC_FAILED, wid);
memcpy(p, data, thislen);
to:
if(!(p = malloc(thislen))) {
GsError(GR_ERROR_MALLOC_FAILED, wid);
return;
}
memcpy(p, data, thislen);
> Another observation, why do we do we allocate a buffer at all. The function
> Last observation, it doesn't look like anybody is freeing the buffer
> allocated in this function.
Ah, this is a result me cocking up when I fixed a bug that Greg pointed
out where the buffer could get freed twice if two clients selected for
CLIENT_DATA events on the same window- in fixing that pretty obscure
problem I added a rather more obvious memory leak. The old way, the data
got copied once in GrSendClientData() and freed when it was used in
GrGetNextEventWrapperFinish() (for every client which had selected for it,
which was the original bug). After the fix, the data gets copied once per
client in GsDeliverClientDataEvent() instead, which fixes the original
bug, but I forgot to take out the (now completely unnecessary) copy in
GrSendClientData(). So forget the previous fix to GrSendClientData(); it
should instead simply be:
void
GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
GR_LENGTH len, GR_LENGTH thislen, void
*data)
{
GsDeliverClientDataEvent(did, wid, serial, len, thislen, data);
}
I just tried transferring 64MB of data through it with this fix, and the
nano-X memory usage stayed reasonable.
--
------- Alex Holden -------
http://www.linuxhacker.org/
http://www.robogeeks.org/
