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?
>
> 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
