Sam, No, I'm not using valgrind but am using a non-head branch that didn't have the patch for the off-by-4 bug in encode_string. In the discussions below it became clear that the fix in the non-valgrind version of encode_string was both correct and also needed in the valgrind version of encode_string.
Michael Sam Lang wrote: > > Michael, > > Are you using valgrind? I'm curious what motivated this commit? > > -sam > > On Sep 4, 2009, at 9:27 AM, Michael Moore wrote: > >> Michael Moore wrote: >>> Pete Wyckoff wrote: >>>> [email protected] wrote on Fri, 04 Sep 2009 09:29 -0400: >>>>> Pete Wyckoff wrote: >>>>>> [email protected] wrote on Thu, 03 Sep 2009 16:31 -0400: >>>>>>> But Pete, the problem is encode_string does not write 8 bytes of >>>>>>> 0, it >>>>>>> writes 4 bytes of 0. If the buffer is 0's to start with what is >>>>>>> here >>>>>>> works fine, but if there is something else in the field you get >>>>>>> garbage. >>>>>>> >>>>>>> Seems like a prudent move to simply zero out all 8 bytes instead >>>>>>> of just >>>>>>> 4, wouldn't you say? >>>>>> Thanks for explaining. I'm looking at the wrong end of the problem, >>>>>> maybe. >>>>>> >>>>>> On the encode side, we have >>>>>> >>>>>> #define encode_string(pptr,pbuf) do { \ >>>>>> u_int32_t len = 0; \ >>>>>> if (*pbuf) \ >>>>>> len = strlen(*pbuf); \ >>>>>> *(u_int32_t *) *(pptr) = htobmi32(len); \ >>>>>> if (len) { \ >>>>>> memcpy(*(pptr)+4, *pbuf, len+1); \ >>>>>> *(pptr) += roundup8(4 + len + 1); \ >>>>>> } else { \ >>>>>> *(u_int32_t *) (*(pptr)+4) = 0; \ >>>>>> *(pptr) += 8; \ >>>>>> } \ >>>>>> } while (0) >>>>> There's the mixup, we're looking at different versions of >>>>> encode_string! >>>>> From a fresh 2.8.1 and the branch I'm working from I have the >>>>> following >>>>> encode_string: >>>>> >>>>> #define encode_string(pptr,pbuf) do { \ >>>>> u_int32_t len = 0; \ >>>>> if (*pbuf) \ >>>>> len = strlen(*pbuf); \ >>>>> *(u_int32_t *) *(pptr) = htobmi32(len); \ >>>>> if (len) { \ >>>>> memcpy(*(pptr)+4, *pbuf, len+1); \ >>>>> int pad = roundup8(4 + len + 1) - (4 + len + 1); \ >>>>> *(pptr) += roundup8(4 + len + 1); \ >>>>> memset(*(pptr)-pad, 0, pad); \ >>>>> } else { \ >>>>> *(u_int32_t *) *(pptr) = 0; \ >>>>> *(pptr) += 8; \ >>>>> } \ >>>>> >>>>> In the else branch there is no +4 in the *pptr assignment. Which >>>>> branch >>>>> are you working from? It looks like this was already addressed >>>>> somewhere >>>>> before? >>>> I go to pvfs.org/fisheye. Looking at cvs vers 1.27 now. I was >>>> quoting the non-valgrind one. Which seems to be different from the >>>> valgrind one you quote above in this important way. My vote goes >>>> toward fixing the valgrind version to fix the off-by-4 bug you just >>>> found. It's old, apparently, but would only hurt if you configured >>>> --with-valgrind. >>>> >>>> Good catch. Looking at that mess of #defines is always a challenge. >>>> >>>> -- Pete >>>> >>> >>> The above snippet is the non-Valgrind version but both versions have the >>> issue in pre-1.27. It looks like the non-valgrind version just got fixed >>> by dbonnie in the 1.26->1.27 patch on Wednesday. >> >> Scratch that, it was a patch back in July by nlmills to specifically >> address this issue. >> >> Michael >>> >>> Thanks for your help! >>> >>> Michael >>> >>>>>> For a NULL pbuf, or a non-NULL pbuf that starts with'\0', >>>>>> we get: >>>>>> >>>>>> len = 0 >>>>>> pptr[0..3] = 0 >>>>>> (take the else clause) >>>>>> pptr[4..7] = 0 >>>>>> (pptr offset up by 8) >>>>>> >>>>>> That generates 8 bytes okay. >>>>>> >>>>>> For any other pbuf with len > 0, say 1 for "a\0", we get: >>>>>> >>>>>> len = 1 >>>>>> pptr[0..3] = htonl(1) >>>>>> (take the if clause) >>>>>> pptr[4..7] = 'a' '\0' 'x' 'x' /* 'x' == garbage */ >>>>>> (pptr offset up by 8) >>>>>> >>>>>> Is it those two bytes of 'x' that you don't like? That's why >>>>>> we have the valgrind ifdef. What am I missing? >>>>>> >>>>>> Michael's initial patch was: >>>>>> >>>>>> 131c131,134 >>>>>> < memcpy(pbuf, *(pptr) + 4, len + 1); \ >>>>>> --- >>>>>>>> if( len ) \ >>>>>>>> memcpy(pbuf, *(pptr) + 4, len + 1); \ >>>>>>>> else \ >>>>>>>> memcpy(pbuf, *(pptr), len + 1); \ >>>>>> >>>>>> Unfortunately without the useful "-u -p" args to diff, but eyeing >>>>>> the source shows he's patching on the decode side. >>>>>> >>>>>> -- Pete >>>>>> >>>>> Apologies for the lack of context on the diff but you're right it >>>>> was on >>>>> the decode side. However, when you mentioned the encode side always >>>>> shipped 8 bytes I looked closer at the encode side and saw in this >>>>> version only 4 of the 8 bytes were getting set to '\0'. >>>>> >>>>> Michael >>>>> >>>>>>> Pete Wyckoff wrote: >>>>>>>> [email protected] wrote on Thu, 03 Sep 2009 13:44 -0400: >>>>>>>>> Pete Wyckoff wrote: >>>>>>>>>> [email protected] wrote on Thu, 03 Sep 2009 11:51 -0400: >>>>>>>>>>> In looking at some issues I was having with the encoding of >>>>>>>>>>> PVFS_dirent >>>>>>>>>>> structs in requests I saw an inconsistency in how >>>>>>>>>>> here_strings are >>>>>>>>>>> encoded and decoded. encode_string memcpys strings starting >>>>>>>>>>> at *(pptr)+4 >>>>>>>>>>> unless it's length 0 in which case it sets *(pptr) to 0. >>>>>>>>>>> However, >>>>>>>>>>> decode_here_string always copys from *(pptr) + 4. So, if >>>>>>>>>>> d_name is an >>>>>>>>>>> empty string when encoded d_name gets 1 byte of *(pptr)+4 >>>>>>>>>>> instead of 0 >>>>>>>>>>> on decoding. >>>>>>>>>>> >>>>>>>>>>> The fix is just to handle decoding like encoding. Is there a >>>>>>>>>>> reason for >>>>>>>>>>> always copying to *(pptr)+4 in decode_here_string? Is this >>>>>>>>>>> something >>>>>>>>>>> that should be changed? >>>>>>>>>> encode_string always ships at least 8 bytes. For a null >>>>>>>>>> string, that's 8 >>>>>>>>>> bytes of zeroes. Decoding a null "here" string will use one of >>>>>>>>>> those zero bytes to set pbuf[0] = '\0'. I figured it would be >>>>>>>>>> nice >>>>>>>>>> to make sure the string was set to NULL. >>>>>>>>>> >>>>>>>>> I see where 8 bytes are always shipped in encode_string but I'm >>>>>>>>> not >>>>>>>>> seeing where 8 bytes of '\0' get encoded for a NULL string or >>>>>>>>> if the >>>>>>>>> length of the string is 0. >>>>>>>>> >>>>>>>>> However, adding an 8 byte memset worth of '\0' to *(pptr) in >>>>>>>>> the else >>>>>>>>> (length 0) branch of encode_string also resolves the problem I was >>>>>>>>> seeing. So, your point may make for a better solution. >>>>>>>> uint8_t *wiredata; >>>>>>>> char mystr[20]; >>>>>>>> decode_here_string(&wiredata, mystr); >>>>>>>> >>>>>>>> wiredata -> 0 0 0 0 0 0 0 0 >>>>>>>> mystr unititialized >>>>>>>> >>>>>>>> 131 memcpy(pbuf, *(pptr) + 4, len + 1); \ >>>>>>>> >>>>>>>> memcpy(pbuf, <bunch of zeroes>, 1) >>>>>>>> equiv to >>>>>>>> >>>>>>>> pbuf[0] = '\0'; >>>>>>>> or >>>>>>>> mystr[0] = '\0'; >>>>>>>> >>>>>>>> right? It's a _here_ string, you don't want to destroy the >>>>>>>> pointer, >>>>>>>> you want to put a zero into it to nullify the string. >>>>>>>> >>>>>>>> The comment does say "odd variation". I don't remember where this >>>>>>>> even gets used. >>>>>>>> >>>>>>>> -- Pete >>>>>>>> _______________________________________________ >>>>>>>> Pvfs2-developers mailing list >>>>>>>> [email protected] >>>>>>>> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers >>>>>>>> >>> >>> _______________________________________________ >>> Pvfs2-developers mailing list >>> [email protected] >>> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers >> >> _______________________________________________ >> Pvfs2-developers mailing list >> [email protected] >> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers > _______________________________________________ Pvfs2-developers mailing list [email protected] http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
