[Pvfs2-developers] here_string endecode bug?
Michael Moore
mtmoore at clemson.edu
Fri Sep 4 10:27:10 EDT 2009
Michael Moore wrote:
> Pete Wyckoff wrote:
>> mtmoore at clemson.edu wrote on Fri, 04 Sep 2009 09:29 -0400:
>>> Pete Wyckoff wrote:
>>>> walt at clemson.edu 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:
>>>>>> mtmoore at clemson.edu wrote on Thu, 03 Sep 2009 13:44 -0400:
>>>>>>> Pete Wyckoff wrote:
>>>>>>>> mtmoore at clemson.edu 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
>>>>>> Pvfs2-developers at beowulf-underground.org
>>>>>> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
>
> _______________________________________________
> Pvfs2-developers mailing list
> Pvfs2-developers at beowulf-underground.org
> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
More information about the Pvfs2-developers
mailing list