* [PATCH] dir: support platforms that require aligned reads
@ 2017-07-16 12:17 René Scharfe
2017-07-16 14:04 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: René Scharfe @ 2017-07-16 12:17 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Duy Nguyen, Jeff King
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 <l.s.r@web.de>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] dir: support platforms that require aligned reads
2017-07-16 12:17 [PATCH] dir: support platforms that require aligned reads René Scharfe
@ 2017-07-16 14:04 ` Jeff King
2017-07-16 15:18 ` René Scharfe
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2017-07-16 14:04 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Duy Nguyen
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dir: support platforms that require aligned reads
2017-07-16 14:04 ` Jeff King
@ 2017-07-16 15:18 ` René Scharfe
2017-07-16 16:03 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: René Scharfe @ 2017-07-16 15:18 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano, Duy Nguyen
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é
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dir: support platforms that require aligned reads
2017-07-16 15:18 ` René Scharfe
@ 2017-07-16 16:03 ` Jeff King
0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2017-07-16 16:03 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Duy Nguyen
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-07-16 16:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-16 12:17 [PATCH] dir: support platforms that require aligned reads René Scharfe
2017-07-16 14:04 ` Jeff King
2017-07-16 15:18 ` René Scharfe
2017-07-16 16:03 ` Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).