[Pvfs2-developers] here_string endecode bug?

[Michael Moore]

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.

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