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@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Thanks