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
