Folks,
Chris Jones is going through looking at all the uses of malloc (since
C++ code that calls malloc makes him very nervous, and not without
cause...). Here are a few more things that seem like they might be a
bug or not portable.
Begin forwarded message:
> From: Chris Jones <[log in to unmask]>
> Date: August 20, 2004 8:20:11 AM EDT
> To: Gregory Sharp <[log in to unmask]>
> Subject: Another xrootd potential problem
>
> Greg,
> I found another potential 'string not terminated' problem.
>
> XrdXrClientWorker.cc line 646
>
> if (dlen) {
> char* buff = (char*) malloc(dlen*sizeof(kXR_char));
> rc = lp->Recv(buff, dlen*sizeof(kXR_char));
>
> // Convert the buffer into the stat structure
> //
> union {long long uuid; struct {long hi; long lo;} id;} Dev;
> char *temp = strtok(buff, (const char*) " ");
>
>
> The problem is strtok requires that the char array (in this case buff)
> is null terminated. As far as I can see, that isn't guaranteed
> programatically (although it might be part of the protocol from
> whatever we are talking with over the 'lp->Recv(...)' call.
>
> For safety, I'd up the malloc by 1 and then force a '\0' at the end of
> the buffer after the transfer
>
> if (dlen) {
> //CDJ: pad buffer by 1 for null character
> char* buff = (char*) malloc((dlen+1)*sizeof(kXR_char));
> rc = lp->Recv(buff, dlen*sizeof(kXR_char));
> buff[rc] = '\0';
>
> // Convert the buffer into the stat structure
> //
> union {long long uuid; struct {long hi; long lo;} id;} Dev;
> char *temp = strtok(buff, (const char*) " ");
I would add, at this point, that if there is some chance this code is
called in a multi-threaded environment that calling strtok_r might be
worth considering. I don't know if it is available on all platforms,
alas.
>
> Looking over this code (actually, I'm checking all uses of malloc) and
> I came across this
>
> XrdXrClientWorker.cc line 921
>
> [Look at the very bottom in the 'switch' statement in the redirect
> case]
>
> /**
> * Receive an error response from the server and put the result into an
> * internal error structure
> *
> * Input: dlen - length of error message to be received from the
> server
> * staus - error type. Can be kXR_redirect, kXR_wait etc.
> * method - method from where the error occured
> *
> * Output: return negative error number
> */
> int XrdXrClientWorker::handleError(kXR_int32 dlen,
> kXR_int16 status,
> char *method)
> {
> ServerResponseBody_Error error;
> int rc = 0;
>
> rc = lp->Recv((char*)&error,
> sizeof(kXR_int32) + (dlen-4)*sizeof(kXR_char));
>
> // Check if message has been received correctly
> //
> if (rc != (int)(sizeof(kXR_int32) + (dlen-4)*sizeof(kXR_char))) {
> XrEroute.Emsg(method,
> (char *)"Error message not received correctly.");
> }
>
> // If error type is kXR_error, print the error message. For kXR_wait
> and
> // kXR_redirect assign the respective variables
> //
> switch(status) {
> case kXR_error: XrEroute.Emsg(method, error.errmsg); break;
> case kXR_wait: waitTime = ntohl(error.errnum); break;
> case kXR_redirect: redirectPort = ntohl(error.errnum);
> redirectHost = (char*) malloc(rc - 3);
> memcpy((char *) redirectHost, error.errmsg, rc - 3);
> *(redirectHost+(rc-4)) = '\0';
> redirect = 1;
> break;
> }
>
> return -status;
> } // handleError
I would prefer to see something like
size_t sz = rc - 3; rather that repeating the subtractions all the
time in the code. The sz-1 subraction will always do the right thing.
That way you only have one place where you need to get the size right
if you need to make the sort of change Chris proposes below.
>
> 1) if redirectHost has ever been set before, then the memory allocated
> to the previous time is lost (we have a memory leak)
> 2) There is a definite assumption about padding between the items in
> the ServerResponseBody_Error struct
>
> struct ServerResponseBody_Error {
> kXR_int32 errnum;
> char errmsg[4096]; // Should be sufficient for every use
> };
> this could lead to a porting problem
> 3) I believe we are copying data 1 beyond the end of the message since
> the 'memcpy' is of length rc-4, even though if you remove the first
> kXR_int32 size (which you assume is size 4) then the remaining errmsg
> would be of length rc-4. Therefore I believe the memcpy should be
> memcpy((char *) redirectHost, error.errmsg, rc - 4);
>
> This actually brings up a question about endian. Is the server
> supposed to always send messages to the client in the clients endian?
> Or is it just assumed that the client and server are the same endian?
--
Gregory J. Sharp email: [log in to unmask]
Wilson Synchrotron Laboratory url:
http://www.lepp.cornell.edu/~gregor
Dryden Rd ph: +1 607 255 4882
Ithaca, NY 14853 fax: +1 607 255 8062
