From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 13 Mar 2014 20:51:32 +0100 Subject: [U-Boot] [PATCH V2 2/3] lib: uuid: add functions to generate UUID version 4 In-Reply-To: <20140313191814.GB16360@bill-the-cat> References: <72629ef58556732156fadf26a2a48be85704f224.1393600504.git.p.marczak@samsung.com> <20140313184124.773EE380EAD@gemini.denx.de> <20140313191814.GB16360@bill-the-cat> Message-ID: <53220C44.7030407@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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