>>> On 1/15/2009 at 8:56 AM, in message <496efa2a020000ac0003a...@lucius.provo.novell.com>, "Brad Nicholes" <bnicho...@novell.com> wrote: >>>> On 1/14/2009 at 3:10 PM, in message > <dbdc3b250901141410l1b0acc6ar2cef894903829...@mail.gmail.com>, "Jesse Becker" > <haw...@gmail.com> wrote: >> On Wed, Jan 14, 2009 at 12:56, Brad Nicholes <bnicho...@novell.com> wrote: >>> Sounds good, can we condense the description below into something that we >> can put into the 3.1 STATUS file so that we can start the process of getting > >> this backported into 3.1? >> >> Done. Committed as r1947. >> >> I suggest that once this is accepted, we release 3.1.2 ASAP. >> >> Also, a big thanks to Spike for finding and tracking this problem down. > > > Spike, > Correct me if I am wrong, but I think we still have a one-off problem > with the patch. > > len = q-p; > /* +1 not needed as q-p is already accounting for that */ > element = malloc(len); > strncpy(element, p, len); > element[len] = '\0'; > > > In the above code suppose "len" is 10 so we malloc() 10 bytes which means > that the array indexing for element is 0-9. Then we strncpy() 10 bytes into > "element" which is OK. However setting element[len] = "\0", IOW element[10] > = "\0" is indexing into the element array 1 byte beyond the actual length of > element. So either the code needs to be element = malloc(len+1) or > element[len-1] = "\0" correct? > > I would suggest that maybe rather than allowing the code to malloc() any > size buffer to match the incoming element length that instead or in addition > we limit the element length as well. Allowing the code to malloc() an > unlimited buffer can also lead to security issues. Since the original code > defined a 256 byte buffer, 256 seems to be a good limit for elements of the > path. It might be a better idea to simply add the limit checking rather than > doing a lot of malloc and free calls. If an element is larger than the > limit, then produce a mal-formed error message and bail out. > > Also I don't think that the current patch will work without the multi-path > patch that was also submitted, without being reworked. The current patch > removes the recursive call to process_path() which means that without the for > loop that was provided in the multi-path patch, only one element of the path > will be processed and the rest will be ignored. The current patch probably > needs to be reworked and separated from the multi-path patch. >
After taking a little closer look at the patch, I think we are OK as far as the recursive call to process_path() is concerned since this case is an error condition and should stop processing rather than continuing in the recursive loop. The other two concerns are still there however. I still think that we are off-by-one in the malloc call. It should be len+1 and I still think that we should limit the malloc to 256 rather than allowing it to be unlimited. Brad ------------------------------------------------------------------------------ This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword _______________________________________________ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
