nanogui: Thread: NULL pointer

This function in srvfunc.c 

void
GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
				GR_LENGTH len, GR_LENGTH thislen, void
*data)
{
	void *p;

	if(!(p = malloc(len))) GsError(GR_ERROR_MALLOC_FAILED, wid);
	memcpy(p, data, thislen);

	GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
}

probably ought to return after the GsError() function call

void
GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
				GR_LENGTH len, GR_LENGTH thislen, void
*data)
{
	void *p;

	if(!(p = malloc(len))) 
	{
		GsError(GR_ERROR_MALLOC_FAILED, wid);
		return;
	}
	memcpy(p, data, thislen);

	GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
}

This function in microwin/src/nanox/srvfunc.c 

void
GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
	GR_LENGTH len, GR_LENGTH thislen, void *data)
{
	void *p;

 	if(!(p = malloc(len))) GsError(GR_ERROR_MALLOC_FAILED, wid);
 	memcpy(p, data, thislen);
 
 	GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
}

Actually on second gander this function seems quite messed up. 

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".

Another observation, why do we do we allocate a buffer at all. The function
GsDeliverClientDataEvent() allocates its own buffer and does another
memcpy() into the new buffer that it creates. It seems that we could save a
copy by passing the original pointer to the second function.

Last observation, it doesn't look like anybody is freeing the buffer
allocated in this function. 

Gary James



On Thu, 01 Feb 2001 21:54:06 Gary James wrote:
> This function in srvfunc.c 
> 
> void
> GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
> 				GR_LENGTH len, GR_LENGTH thislen, void
> *data)
> {
> 	void *p;
> 
> 	if(!(p = malloc(len))) GsError(GR_ERROR_MALLOC_FAILED, wid);
> 	memcpy(p, data, thislen);
> 
> 	GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
> }
> 
> probably ought to return after the GsError() function call
> 
> void
> GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
> 				GR_LENGTH len, GR_LENGTH thislen, void
> *data)
> {
> 	void *p;
> 
> 	if(!(p = malloc(len))) 
> 	{
> 		GsError(GR_ERROR_MALLOC_FAILED, wid);
> 		return;
> 	}
> 	memcpy(p, data, thislen);
> 
> 	GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
> }
> 
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: ####@####.####
> For additional commands, e-mail: ####@####.####
> 
> 

: void
: GrSendClientData(GR_WINDOW_ID wid, GR_WINDOW_ID did, GR_SERIALNO serial,
: GR_LENGTH len, GR_LENGTH thislen, void *data)
: {
: void *p;
: 
:   if(!(p = malloc(len))) GsError(GR_ERROR_MALLOC_FAILED, wid);
:   memcpy(p, data, thislen);
:  
:   GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
: }

Yes, you're right about the return, it should return after the malloc failed...


: 
: Actually on second gander this function seems quite messed up. 
: 
: 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".
: 
: Another observation, why do we do we allocate a buffer at all. The function
: GsDeliverClientDataEvent() allocates its own buffer and does another
: memcpy() into the new buffer that it creates. It seems that we could save a
: copy by passing the original pointer to the second function.

I included this function, written by Alex, without any testing, as I was trying
to get pre7 out, and I had been sitting on the patch for a month.  I think
you're generally correct in your observations.  I had asked Alex to test
the get/setselection client test programs, but haven't heard back yet.
The copy is required in the LINK_APP_TO_SERVER case, though.

I am planning on enhancing and generalizing this submission to include
general client-to-client communications, but haven't had a chance yet.

Regards,

Greg

Greg Haerr wrote:


> The copy is required in the LINK_APP_TO_SERVER case, though.

I agree that you need a copy, but the required copy is the function 
GsDeliverClientDataEvent() not in this function.

So how was the show in NYC last week. I was real tempted to drive down and 
visit you but it was way too busy of a week at work. Maybe next year.

Gary James

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/