Hi Gregory,
Once again, thanks. The memory leak has already been fixed (uncovered by
one of your previous comments -- thanks again). The other issues look
problematic as well and will be fixed. BTW please thank Chris as well.
Andy
On Fri, 20 Aug 2004, Gregory J. Sharp wrote:
> 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
>
>
