# [PATCH] dir: support platforms that require aligned reads 4 messages from 2017-07-16 to 2017-07-16. Participants: René Scharfe, Jeff King. Thread: https://gitlist.dev/t/46399 ## René Scharfe, 2017-07-16 12:17 Subject: [PATCH] dir: support platforms that require aligned reads Message-ID: URL: https://gitlist.dev/e/d3db2984-f238-7166-affa-f1f7df566404%40web.de ``` The untracked cache is stored on disk by concatenating its memory structures without any padding. Consequently some of the structs are not aligned at a particular boundary when the whole extension is read back in one go. That's only OK on platforms without strict alignment requirements, or for byte-aligned data like strings or hash values. Decode struct ondisk_untracked_cache carefully from the extension blob by using explicit pointer arithmetic with offsets, avoiding alignment issues. Use char pointers for passing stat_data objects to stat_data_from_disk(), and use memcpy(3) in that function to get the contents into a properly aligned struct, then perform the byte-order adjustment in place there. Found with Clang's UBSan. Signed-off-by: Rene Scharfe --- Side note: The OS name is not enough for determining the layout of struct ondisk_untracked_cache. Different platforms can have different int sizes and padding. Adding the machine type could help, but that would be a breaking change. At that point we would be better off defining a machine-independent format, no? dir.c | 50 +++++++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/dir.c b/dir.c index ae6f5c9636..1c55dc3e36 100644 --- a/dir.c +++ b/dir.c @@ -2398,7 +2398,8 @@ struct ondisk_untracked_cache { char exclude_per_dir[FLEX_ARRAY]; }; -#define ouc_size(len) (offsetof(struct ondisk_untracked_cache, exclude_per_dir) + len + 1) +#define ouc_offset(x) offsetof(struct ondisk_untracked_cache, x) +#define ouc_size(len) (ouc_offset(exclude_per_dir) + len + 1) struct write_data { int index; /* number of written untracked_cache_dir */ @@ -2560,17 +2561,18 @@ struct read_data { const unsigned char *end; }; -static void stat_data_from_disk(struct stat_data *to, const struct stat_data *from) +static void stat_data_from_disk(struct stat_data *to, const unsigned char *data) { - to->sd_ctime.sec = get_be32(&from->sd_ctime.sec); - to->sd_ctime.nsec = get_be32(&from->sd_ctime.nsec); - to->sd_mtime.sec = get_be32(&from->sd_mtime.sec); - to->sd_mtime.nsec = get_be32(&from->sd_mtime.nsec); - to->sd_dev = get_be32(&from->sd_dev); - to->sd_ino = get_be32(&from->sd_ino); - to->sd_uid = get_be32(&from->sd_uid); - to->sd_gid = get_be32(&from->sd_gid); - to->sd_size = get_be32(&from->sd_size); + memcpy(to, data, sizeof(*to)); + to->sd_ctime.sec = ntohl(to->sd_ctime.sec); + to->sd_ctime.nsec = ntohl(to->sd_ctime.nsec); + to->sd_mtime.sec = ntohl(to->sd_mtime.sec); + to->sd_mtime.nsec = ntohl(to->sd_mtime.nsec); + to->sd_dev = ntohl(to->sd_dev); + to->sd_ino = ntohl(to->sd_ino); + to->sd_uid = ntohl(to->sd_uid); + to->sd_gid = ntohl(to->sd_gid); + to->sd_size = ntohl(to->sd_size); } static int read_one_dir(struct untracked_cache_dir **untracked_, @@ -2645,7 +2647,7 @@ static void read_stat(size_t pos, void *cb) rd->data = rd->end + 1; return; } - stat_data_from_disk(&ud->stat_data, (struct stat_data *)rd->data); + stat_data_from_disk(&ud->stat_data, rd->data); rd->data += sizeof(struct stat_data); ud->valid = 1; } @@ -2663,22 +2665,22 @@ static void read_sha1(size_t pos, void *cb) } static void load_sha1_stat(struct sha1_stat *sha1_stat, - const struct stat_data *stat, + const unsigned char *data, const unsigned char *sha1) { - stat_data_from_disk(&sha1_stat->stat, stat); + stat_data_from_disk(&sha1_stat->stat, data); hashcpy(sha1_stat->sha1, sha1); sha1_stat->valid = 1; } struct untracked_cache *read_untracked_extension(const void *data, unsigned long sz) { - const struct ondisk_untracked_cache *ouc; struct untracked_cache *uc; struct read_data rd; const unsigned char *next = data, *end = (const unsigned char *)data + sz; const char *ident; int ident_len, len; + const char *exclude_per_dir; if (sz <= 1 || end[-1] != '\0') return NULL; @@ -2690,21 +2692,23 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long ident = (const char *)next; next += ident_len; - ouc = (const struct ondisk_untracked_cache *)next; if (next + ouc_size(0) > end) return NULL; uc = xcalloc(1, sizeof(*uc)); strbuf_init(&uc->ident, ident_len); strbuf_add(&uc->ident, ident, ident_len); - load_sha1_stat(&uc->ss_info_exclude, &ouc->info_exclude_stat, - ouc->info_exclude_sha1); - load_sha1_stat(&uc->ss_excludes_file, &ouc->excludes_file_stat, - ouc->excludes_file_sha1); - uc->dir_flags = get_be32(&ouc->dir_flags); - uc->exclude_per_dir = xstrdup(ouc->exclude_per_dir); + load_sha1_stat(&uc->ss_info_exclude, + next + ouc_offset(info_exclude_stat), + next + ouc_offset(info_exclude_sha1)); + load_sha1_stat(&uc->ss_excludes_file, + next + ouc_offset(excludes_file_stat), + next + ouc_offset(excludes_file_sha1)); + uc->dir_flags = get_be32(next + ouc_offset(dir_flags)); + exclude_per_dir = (const char *)next + ouc_offset(exclude_per_dir); + uc->exclude_per_dir = xstrdup(exclude_per_dir); /* NUL after exclude_per_dir is covered by sizeof(*ouc) */ - next += ouc_size(strlen(ouc->exclude_per_dir)); + next += ouc_size(strlen(exclude_per_dir)); if (next >= end) goto done2; -- 2.13.3 ``` ## Jeff King, 2017-07-16 14:04 Subject: Re: [PATCH] dir: support platforms that require aligned reads Message-ID: <20170716140409.3ywepgvo5c6ognsy@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170716140409.3ywepgvo5c6ognsy%40sigill.intra.peff.net In-Reply-To: ``` On Sun, Jul 16, 2017 at 02:17:37PM +0200, René Scharfe wrote: > -static void stat_data_from_disk(struct stat_data *to, const struct stat_data *from) > +static void stat_data_from_disk(struct stat_data *to, const unsigned char *data) > { > - to->sd_ctime.sec = get_be32(&from->sd_ctime.sec); > - to->sd_ctime.nsec = get_be32(&from->sd_ctime.nsec); > - to->sd_mtime.sec = get_be32(&from->sd_mtime.sec); > - to->sd_mtime.nsec = get_be32(&from->sd_mtime.nsec); > - to->sd_dev = get_be32(&from->sd_dev); > - to->sd_ino = get_be32(&from->sd_ino); > - to->sd_uid = get_be32(&from->sd_uid); > - to->sd_gid = get_be32(&from->sd_gid); > - to->sd_size = get_be32(&from->sd_size); > + memcpy(to, data, sizeof(*to)); > + to->sd_ctime.sec = ntohl(to->sd_ctime.sec); > + to->sd_ctime.nsec = ntohl(to->sd_ctime.nsec); > + to->sd_mtime.sec = ntohl(to->sd_mtime.sec); > + to->sd_mtime.nsec = ntohl(to->sd_mtime.nsec); > + to->sd_dev = ntohl(to->sd_dev); > + to->sd_ino = ntohl(to->sd_ino); > + to->sd_uid = ntohl(to->sd_uid); > + to->sd_gid = ntohl(to->sd_gid); > + to->sd_size = ntohl(to->sd_size); > } Hmm. I would have written this to pull the bytes directly out of the array, like: to->sd_ctime.sec = get_be32(data); data += 4; to->sd_ctime.nsec = get_be32(data); data += 4; etc. Or even a helper to do the advancing like: to->sd_ctime.sec = parse_be32(&data); That reduces assumptions about padding in "struct stat_data". But looking more at this code, and reading your comment: > Side note: The OS name is not enough for determining the layout of > struct ondisk_untracked_cache. Different platforms can have different > int sizes and padding. Adding the machine type could help, but that > would be a breaking change. At that point we would be better off > defining a machine-independent format, no? it looks like assumptions about struct layout are pervasive and part of the on-disk format. Yuck. :( -Peff ``` ## René Scharfe, 2017-07-16 15:18 Subject: Re: [PATCH] dir: support platforms that require aligned reads Message-ID: <84b4d5aa-956f-932b-4e95-f181243215bc@web.de> URL: https://gitlist.dev/e/84b4d5aa-956f-932b-4e95-f181243215bc%40web.de In-Reply-To: <20170716140409.3ywepgvo5c6ognsy@sigill.intra.peff.net> ``` Am 16.07.2017 um 16:04 schrieb Jeff King: > On Sun, Jul 16, 2017 at 02:17:37PM +0200, René Scharfe wrote: > >> -static void stat_data_from_disk(struct stat_data *to, const struct stat_data *from) >> +static void stat_data_from_disk(struct stat_data *to, const unsigned char *data) >> { >> - to->sd_ctime.sec = get_be32(&from->sd_ctime.sec); >> - to->sd_ctime.nsec = get_be32(&from->sd_ctime.nsec); >> - to->sd_mtime.sec = get_be32(&from->sd_mtime.sec); >> - to->sd_mtime.nsec = get_be32(&from->sd_mtime.nsec); >> - to->sd_dev = get_be32(&from->sd_dev); >> - to->sd_ino = get_be32(&from->sd_ino); >> - to->sd_uid = get_be32(&from->sd_uid); >> - to->sd_gid = get_be32(&from->sd_gid); >> - to->sd_size = get_be32(&from->sd_size); >> + memcpy(to, data, sizeof(*to)); >> + to->sd_ctime.sec = ntohl(to->sd_ctime.sec); >> + to->sd_ctime.nsec = ntohl(to->sd_ctime.nsec); >> + to->sd_mtime.sec = ntohl(to->sd_mtime.sec); >> + to->sd_mtime.nsec = ntohl(to->sd_mtime.nsec); >> + to->sd_dev = ntohl(to->sd_dev); >> + to->sd_ino = ntohl(to->sd_ino); >> + to->sd_uid = ntohl(to->sd_uid); >> + to->sd_gid = ntohl(to->sd_gid); >> + to->sd_size = ntohl(to->sd_size); >> } > > Hmm. I would have written this to pull the bytes directly out of the > array, like: > > to->sd_ctime.sec = get_be32(data); data += 4; > to->sd_ctime.nsec = get_be32(data); data += 4; > > etc. Or even a helper to do the advancing like: > > to->sd_ctime.sec = parse_be32(&data); > > That reduces assumptions about padding in "struct stat_data". But > looking more at this code, and reading your comment: > >> Side note: The OS name is not enough for determining the layout of >> struct ondisk_untracked_cache. Different platforms can have different >> int sizes and padding. Adding the machine type could help, but that >> would be a breaking change. At that point we would be better off >> defining a machine-independent format, no? > > it looks like assumptions about struct layout are pervasive and part of > the on-disk format. Yuck. :( Assuming that there is no padding probably even works for the platforms the code currently supports (basically x86), but I don't know about others. We'd need to change the writing side as well to match, though. Which is probably a good idea, but I tried to keep the patch small and its impact low. Cross-machine usability is currently explicitly not supported -- not sure why, though. René ``` ## Jeff King, 2017-07-16 16:03 Subject: Re: [PATCH] dir: support platforms that require aligned reads Message-ID: <20170716160313.2crnw6exyqpvp7ue@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170716160313.2crnw6exyqpvp7ue%40sigill.intra.peff.net In-Reply-To: <84b4d5aa-956f-932b-4e95-f181243215bc@web.de> ``` On Sun, Jul 16, 2017 at 05:18:27PM +0200, René Scharfe wrote: > > it looks like assumptions about struct layout are pervasive and part of > > the on-disk format. Yuck. :( > > Assuming that there is no padding probably even works for the platforms > the code currently supports (basically x86), but I don't know about > others. We'd need to change the writing side as well to match, though. > Which is probably a good idea, but I tried to keep the patch small and > its impact low. Cross-machine usability is currently explicitly not > supported -- not sure why, though. Yeah, I think your patch is a good first step, and we don't have to go further for now. -Peff ```