[U-Boot] [PATCH V2 2/3] lib: uuid: add functions to generate UUID version 4

[Przemyslaw Marczak]

Hello,

On 03/13/2014 08:18 PM, Tom Rini wrote:
> On Thu, Mar 13, 2014 at 07:41:24PM +0100, Wolfgang Denk wrote:
>> Dear Przemyslaw Marczak,
>>
>> In message <cc0f558724a4d3ea3497b84601038f5f18f37a7b.1394037321.git.p.marczak at samsung.com> you wrote:
>>> This patch adds support to generate UUID (Universally Unique Identifier)
>>> in version 4 based on RFC4122, which is randomly.
>> ...
>>> +struct uuid {
>>> +	unsigned int time_low;
>>> +	unsigned short time_mid;
>>> +	unsigned short time_hi_and_version;
>>> +	unsigned char clock_seq_hi_and_reserved;
>>> +	unsigned char clock_seq_low;
>>> +	unsigned char node[6];
>>> +};
>>
>> This struct starts with an uint, so it requires alignment on a 32 bit
>> boundary (i. e. an address that is a multiple of 4).
>
> And this needs to be marked as packed since we're using this as a direct
> representation of things on-disk.
>

ok, I will add packed attribute.

>>> +void gen_rand_uuid(unsigned char *uuid_bin)
I can change this pointer above to unsigned int.

>>> +{
>>> +	struct uuid *uuid = (struct uuid *)uuid_bin;
>>
>> Here you cast a pointer to the (unaligned) character buffer to a
>> struct buffer, which requires alignment.
>
> Potentially unaligned buffer.  There's only one caller thus far, and it
> will be aligned there.  We do need to comment that the pointer needs to
> be aligned.
>
>>> +	unsigned int *ptr = (unsigned int *)uuid_bin;
>>
>>> +	/* Set all fields randomly */
>>> +	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
>>> +		*(ptr + i) = rand();
>>
>> This code is dangerous - if the size of the struct should not be a
>> multiple of sizeof(uint), there would remain uninitialized data.
>

We know that uuid is 16bytes, so change the divider to "4" is better 
solution?

> With the struct not packed, it'll be padded out so this works.  But
> looking at how we later use this as I say above, we do need to pack it,
> and then this will not be safe.  Some looping of strncpy into the char
> buffer, as a char so we whack the rand data in?
>
>>> +	/* Set V4 format */
>>> +	uuid->time_hi_and_version &= UUID_VERSION_CLEAR_BITS;
>>> +	uuid->time_hi_and_version |= UUID_VERSION << UUID_VERSION_SHIFT;
>>
>> Potentially unaligned accesses.
Ok, I change this to clrsetbits.

>
> As-is no (hidden padding), with packed yes-but-handled (compiler can see
> this too, will behave correctly).
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

Thanks
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com