nanogui: Thread: Possible bug in GrSendClientData?

I was learning how to use GrSendClientData() by going through the code 
and I may have spotted a possible bug.

You call GrSendClient() with a pointer to a buffer and within 
GrSendClient() you alloc memory for variable 'p' and copy the data from 
the user's data buffer to the new buffer pointed to by 'p', and then 
GsDeliverClientDataEvent()  is called passing 'p'.
Now in GsDeliverClientDataEvent() a GR_EVENT_CLIENT_DATA struct is used. 
It has as a member called  gp->data. Another malloc is done using 
gp->data, and ANOTHER memcpy is done from the area pointed to by 'p' 
from GrSendClient() to this new 'data' area.

Where is the memory allocated to 'p' in GrSendClient() free'ed? There is 
no free in GrSendClient() after the call to GsDeliverClientDataEvent().

I have yet to comprehend the window event handling and I guess the 
mallocs done to gp->data in GsDeliverClientDataEvent() are free'd 
somewhere else. But, 'p' is visible only to GrSendClient(), is passed as 
a parameter only as far as GsDeliverClientDataEvent() and neither one 
frees it.

I have limited knowledge of the nano-X code, but this looks like a 
memory leak to me. Is it? Shouldn't GrSendClient() code look like this?
 
   memcpy(p, data, thislen);

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

    SERVER_UNLOCK();

Plus, I just noticed, in GrSendClient() memory of size 'len' is 
malloc'ed for p>b But, 'thislen' is used for the size of the memcpy. 
Shouldn't the malloc of 'p' be sized to 'thislen'?

tj

Note: This may be a second post of this. I never saw the original come 
back. If this is the second to some, I apologize. I did add a line 
toward the bottom.

I was learning how to use GrSendClientData() by going through the code 
and I may have spotted a possible bug.

You call GrSendClient() with a pointer to a buffer and within 
GrSendClient() you alloc memory for variable 'p' and copy the data from 
the user's data buffer to the new buffer pointed to by 'p', and then 
GsDeliverClientDataEvent()  is called passing 'p'.
Now in GsDeliverClientDataEvent() a GR_EVENT_CLIENT_DATA struct is used. 
It has as a member called  gp->data. Another malloc is done using 
gp->data, and ANOTHER memcpy is done from the area pointed to by 'p' 
from GrSendClient() to this new 'data' area.

Where is the memory allocated to 'p' in GrSendClient() free'ed? There is 
no free in GrSendClient() after the call to GsDeliverClientDataEvent().

I have yet to comprehend the window event handling and I guess the 
mallocs done to gp->data in GsDeliverClientDataEvent() are free'd 
somewhere else. But, 'p' is visible only to GrSendClient(), is passed as 
a parameter only as far as GsDeliverClientDataEvent() and neither one 
frees it.

I have limited knowledge of the nano-X code, but this looks like a 
memory leak to me. Is it? Shouldn't GrSendClient() code look like this?

  memcpy(p, data, thislen);

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

   SERVER_UNLOCK();

Or, not even create and malloc 'p'. Just pass user supplied 'data' 
pointer, since the GsDeliverClientDataEvent() func copies its contents 
anyway.

Plus, I just noticed, in GrSendClient() memory of size 'len' is 
malloc'ed for p>b But, 'thislen' is used for the size of the memcpy. 
Shouldn't the malloc of 'p' be sized to 'thislen'?

tj

> Where is the memory allocated to 'p' in GrSendClient() free'ed? There is 
> no free in GrSendClient() after the call to GsDeliverClientDataEvent().

I've reviewed the code, and yes, there's a big memory leak.


> I have limited knowledge of the nano-X code, but this looks like a 
> memory leak to me. Is it? Shouldn't GrSendClient() code look like this?
>  
>    memcpy(p, data, thislen);
> 
>     GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
>      free(p);
> 
>     SERVER_UNLOCK();

Actually, the GrSendClientData function shouldn't even malloc
data, since only calls GsDeliverClientDataEvent which mallocs
and copies again.  Thus, the correct GrSendClientData implementation
should look like this:

SERVER_LOCK();
GsDeliverClientDataEvent(did, wid, serial, len, thislen, data);
SERVER_UNLOCK();


> Plus, I just noticed, in GrSendClient() memory of size 'len' is 
> malloc'ed for p>b But, 'thislen' is used for the size of the memcpy. 
> Shouldn't the malloc of 'p' be sized to 'thislen'?

Yep - but this isn't needed anymore with the above implementation.

Regards,

Greg

Cool,

I'll make the change in my nano-X tree and I guess you will make the 
change in CVs once you get a system up and running again.

Also, I assume that the malloc's done for the GsDeliverClientDataEvent() 
are the pointers that are delivered to the recipient, and the recipient, 
ie destination window id, application code is responsible for free'ing 
it, etc

tj

Greg Haerr wrote:

>>Where is the memory allocated to 'p' in GrSendClient() free'ed? There is 
>>no free in GrSendClient() after the call to GsDeliverClientDataEvent().
>>    
>>
>
>I've reviewed the code, and yes, there's a big memory leak.
>
>
>  
>
>>I have limited knowledge of the nano-X code, but this looks like a 
>>memory leak to me. Is it? Shouldn't GrSendClient() code look like this?
>> 
>>   memcpy(p, data, thislen);
>>
>>    GsDeliverClientDataEvent(did, wid, serial, len, thislen, p);
>>     free(p);
>>
>>    SERVER_UNLOCK();
>>    
>>
>
>Actually, the GrSendClientData function shouldn't even malloc
>data, since only calls GsDeliverClientDataEvent which mallocs
>and copies again.  Thus, the correct GrSendClientData implementation
>should look like this:
>
>SERVER_LOCK();
>GsDeliverClientDataEvent(did, wid, serial, len, thislen, data);
>SERVER_UNLOCK();
>
>
>  
>
>>Plus, I just noticed, in GrSendClient() memory of size 'len' is 
>>malloc'ed for p>b But, 'thislen' is used for the size of the memcpy. 
>>Shouldn't the malloc of 'p' be sized to 'thislen'?
>>    
>>
>
>Yep - but this isn't needed anymore with the above implementation.
>
>Regards,
>
>Greg
>
>
>  
>

> Also, I assume that the malloc's done for the GsDeliverClientDataEvent() 
> are the pointers that are delivered to the recipient, and the recipient, 
> ie destination window id, application code is responsible for free'ing 
> it, etc

Well, almost.  In the client/server environment, the client
data that is malloc'd by the server is special-cased, and
written on the client socket after the event, and then freeed
by the server in nanox/srvnet.c::GrGetNextEventWrapperFinish().

The client data space is malloc'd for again by the client in
nanox/client.c::CheckForClientData().  Finally, the client is
responsible for freeing that malloc, but the pointers aren't
the same.  This above code is not required for the
LINK_APP_INTO_SERVER case, when your
above comments are correct.

Regards,

Greg