[Pvfs2-developers] here_string endecode bug?
Sam Lang
slang at mcs.anl.gov
Tue Sep 8 10:20:11 EDT 2009
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:
>>> 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
>
> _______________________________________________
> 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