{"thread":{"id":"46399","subject":"[PATCH] dir: support platforms that require aligned reads","startedAt":"2017-07-16T12:17:59Z","lastAt":"2017-07-16T16:03:22Z","messageCount":4,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324620","messageId":"d3db2984-f238-7166-affa-f1f7df566404@web.de","threadId":"46399","inReplyTo":null,"subject":"[PATCH] dir: support platforms that require aligned reads","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T12:17:37Z","receivedAt":"2017-07-16T12:17:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The untracked cache is stored on disk by concatenating its memory\nstructures without any padding.  Consequently some of the structs are\nnot aligned at a particular boundary when the whole extension is read\nback in one go.  That's only OK on platforms without strict alignment\nrequirements, or for byte-aligned data like strings or hash values.\n\nDecode struct ondisk_untracked_cache carefully from the extension\nblob by using explicit pointer arithmetic with offsets, avoiding\nalignment issues.  Use char pointers for passing stat_data objects to\nstat_data_from_disk(), and use memcpy(3) in that function to  get the\ncontents into a properly aligned struct, then perform the byte-order\nadjustment in place there.\n\nFound with Clang's UBSan.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nSide note: The OS name is not enough for determining the layout of\nstruct ondisk_untracked_cache.  Different platforms can have\ndifferent int sizes and padding.  Adding the machine type could\nhelp, but that would be a breaking change.  At that point we would\nbe better off defining a machine-independent format, no?\n\n dir.c | 50 +++++++++++++++++++++++++++-----------------------\n 1 file changed, 27 insertions(+), 23 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ae6f5c9636..1c55dc3e36 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2398,7 +2398,8 @@ struct ondisk_untracked_cache {\n \tchar exclude_per_dir[FLEX_ARRAY];\n };\n \n-#define ouc_size(len) (offsetof(struct ondisk_untracked_cache, exclude_per_dir) + len + 1)\n+#define ouc_offset(x) offsetof(struct ondisk_untracked_cache, x)\n+#define ouc_size(len) (ouc_offset(exclude_per_dir) + len + 1)\n \n struct write_data {\n \tint index;\t   /* number of written untracked_cache_dir */\n@@ -2560,17 +2561,18 @@ struct read_data {\n \tconst unsigned char *end;\n };\n \n-static void stat_data_from_disk(struct stat_data *to, const struct stat_data *from)\n+static void stat_data_from_disk(struct stat_data *to, const unsigned char *data)\n {\n-\tto->sd_ctime.sec  = get_be32(&from->sd_ctime.sec);\n-\tto->sd_ctime.nsec = get_be32(&from->sd_ctime.nsec);\n-\tto->sd_mtime.sec  = get_be32(&from->sd_mtime.sec);\n-\tto->sd_mtime.nsec = get_be32(&from->sd_mtime.nsec);\n-\tto->sd_dev\t  = get_be32(&from->sd_dev);\n-\tto->sd_ino\t  = get_be32(&from->sd_ino);\n-\tto->sd_uid\t  = get_be32(&from->sd_uid);\n-\tto->sd_gid\t  = get_be32(&from->sd_gid);\n-\tto->sd_size\t  = get_be32(&from->sd_size);\n+\tmemcpy(to, data, sizeof(*to));\n+\tto->sd_ctime.sec  = ntohl(to->sd_ctime.sec);\n+\tto->sd_ctime.nsec = ntohl(to->sd_ctime.nsec);\n+\tto->sd_mtime.sec  = ntohl(to->sd_mtime.sec);\n+\tto->sd_mtime.nsec = ntohl(to->sd_mtime.nsec);\n+\tto->sd_dev\t  = ntohl(to->sd_dev);\n+\tto->sd_ino\t  = ntohl(to->sd_ino);\n+\tto->sd_uid\t  = ntohl(to->sd_uid);\n+\tto->sd_gid\t  = ntohl(to->sd_gid);\n+\tto->sd_size\t  = ntohl(to->sd_size);\n }\n \n static int read_one_dir(struct untracked_cache_dir **untracked_,\n@@ -2645,7 +2647,7 @@ static void read_stat(size_t pos, void *cb)\n \t\trd->data = rd->end + 1;\n \t\treturn;\n \t}\n-\tstat_data_from_disk(&ud->stat_data, (struct stat_data *)rd->data);\n+\tstat_data_from_disk(&ud->stat_data, rd->data);\n \trd->data += sizeof(struct stat_data);\n \tud->valid = 1;\n }\n@@ -2663,22 +2665,22 @@ static void read_sha1(size_t pos, void *cb)\n }\n \n static void load_sha1_stat(struct sha1_stat *sha1_stat,\n-\t\t\t   const struct stat_data *stat,\n+\t\t\t   const unsigned char *data,\n \t\t\t   const unsigned char *sha1)\n {\n-\tstat_data_from_disk(&sha1_stat->stat, stat);\n+\tstat_data_from_disk(&sha1_stat->stat, data);\n \thashcpy(sha1_stat->sha1, sha1);\n \tsha1_stat->valid = 1;\n }\n \n struct untracked_cache *read_untracked_extension(const void *data, unsigned long sz)\n {\n-\tconst struct ondisk_untracked_cache *ouc;\n \tstruct untracked_cache *uc;\n \tstruct read_data rd;\n \tconst unsigned char *next = data, *end = (const unsigned char *)data + sz;\n \tconst char *ident;\n \tint ident_len, len;\n+\tconst char *exclude_per_dir;\n \n \tif (sz <= 1 || end[-1] != '\\0')\n \t\treturn NULL;\n@@ -2690,21 +2692,23 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long\n \tident = (const char *)next;\n \tnext += ident_len;\n \n-\touc = (const struct ondisk_untracked_cache *)next;\n \tif (next + ouc_size(0) > end)\n \t\treturn NULL;\n \n \tuc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, ident_len);\n \tstrbuf_add(&uc->ident, ident, ident_len);\n-\tload_sha1_stat(&uc->ss_info_exclude, &ouc->info_exclude_stat,\n-\t\t       ouc->info_exclude_sha1);\n-\tload_sha1_stat(&uc->ss_excludes_file, &ouc->excludes_file_stat,\n-\t\t       ouc->excludes_file_sha1);\n-\tuc->dir_flags = get_be32(&ouc->dir_flags);\n-\tuc->exclude_per_dir = xstrdup(ouc->exclude_per_dir);\n+\tload_sha1_stat(&uc->ss_info_exclude,\n+\t\t       next + ouc_offset(info_exclude_stat),\n+\t\t       next + ouc_offset(info_exclude_sha1));\n+\tload_sha1_stat(&uc->ss_excludes_file,\n+\t\t       next + ouc_offset(excludes_file_stat),\n+\t\t       next + ouc_offset(excludes_file_sha1));\n+\tuc->dir_flags = get_be32(next + ouc_offset(dir_flags));\n+\texclude_per_dir = (const char *)next + ouc_offset(exclude_per_dir);\n+\tuc->exclude_per_dir = xstrdup(exclude_per_dir);\n \t/* NUL after exclude_per_dir is covered by sizeof(*ouc) */\n-\tnext += ouc_size(strlen(ouc->exclude_per_dir));\n+\tnext += ouc_size(strlen(exclude_per_dir));\n \tif (next >= end)\n \t\tgoto done2;\n \n-- \n2.13.3\n"},{"id":"324623","messageId":"20170716140409.3ywepgvo5c6ognsy@sigill.intra.peff.net","threadId":"46399","inReplyTo":"d3db2984-f238-7166-affa-f1f7df566404@web.de","subject":"Re: [PATCH] dir: support platforms that require aligned reads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-16T14:04:09Z","receivedAt":"2017-07-16T14:04:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 16, 2017 at 02:17:37PM +0200, René Scharfe wrote:\n\n> -static void stat_data_from_disk(struct stat_data *to, const struct stat_data *from)\n> +static void stat_data_from_disk(struct stat_data *to, const unsigned char *data)\n>  {\n> -\tto->sd_ctime.sec  = get_be32(&from->sd_ctime.sec);\n> -\tto->sd_ctime.nsec = get_be32(&from->sd_ctime.nsec);\n> -\tto->sd_mtime.sec  = get_be32(&from->sd_mtime.sec);\n> -\tto->sd_mtime.nsec = get_be32(&from->sd_mtime.nsec);\n> -\tto->sd_dev\t  = get_be32(&from->sd_dev);\n> -\tto->sd_ino\t  = get_be32(&from->sd_ino);\n> -\tto->sd_uid\t  = get_be32(&from->sd_uid);\n> -\tto->sd_gid\t  = get_be32(&from->sd_gid);\n> -\tto->sd_size\t  = get_be32(&from->sd_size);\n> +\tmemcpy(to, data, sizeof(*to));\n> +\tto->sd_ctime.sec  = ntohl(to->sd_ctime.sec);\n> +\tto->sd_ctime.nsec = ntohl(to->sd_ctime.nsec);\n> +\tto->sd_mtime.sec  = ntohl(to->sd_mtime.sec);\n> +\tto->sd_mtime.nsec = ntohl(to->sd_mtime.nsec);\n> +\tto->sd_dev\t  = ntohl(to->sd_dev);\n> +\tto->sd_ino\t  = ntohl(to->sd_ino);\n> +\tto->sd_uid\t  = ntohl(to->sd_uid);\n> +\tto->sd_gid\t  = ntohl(to->sd_gid);\n> +\tto->sd_size\t  = ntohl(to->sd_size);\n>  }\n\nHmm. I would have written this to pull the bytes directly out of the\narray, like:\n\n  to->sd_ctime.sec  = get_be32(data); data += 4;\n  to->sd_ctime.nsec = get_be32(data); data += 4;\n\netc. Or even a helper to do the advancing like:\n\n  to->sd_ctime.sec = parse_be32(&data);\n\nThat reduces assumptions about padding in \"struct stat_data\". But\nlooking more at this code, and reading your comment:\n\n> Side note: The OS name is not enough for determining the layout of\n> struct ondisk_untracked_cache.  Different platforms can have different\n> int sizes and padding.  Adding the machine type could help, but that\n> would be a breaking change.  At that point we would be better off\n> defining a machine-independent format, no?\n\nit looks like assumptions about struct layout are pervasive and part of\nthe on-disk format. Yuck. :(\n\n-Peff\n"},{"id":"324626","messageId":"84b4d5aa-956f-932b-4e95-f181243215bc@web.de","threadId":"46399","inReplyTo":"20170716140409.3ywepgvo5c6ognsy@sigill.intra.peff.net","subject":"Re: [PATCH] dir: support platforms that require aligned reads","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T15:18:27Z","receivedAt":"2017-07-16T15:18:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.2017 um 16:04 schrieb Jeff King:\n> On Sun, Jul 16, 2017 at 02:17:37PM +0200, René Scharfe wrote:\n> \n>> -static void stat_data_from_disk(struct stat_data *to, const struct stat_data *from)\n>> +static void stat_data_from_disk(struct stat_data *to, const unsigned char *data)\n>>   {\n>> -\tto->sd_ctime.sec  = get_be32(&from->sd_ctime.sec);\n>> -\tto->sd_ctime.nsec = get_be32(&from->sd_ctime.nsec);\n>> -\tto->sd_mtime.sec  = get_be32(&from->sd_mtime.sec);\n>> -\tto->sd_mtime.nsec = get_be32(&from->sd_mtime.nsec);\n>> -\tto->sd_dev\t  = get_be32(&from->sd_dev);\n>> -\tto->sd_ino\t  = get_be32(&from->sd_ino);\n>> -\tto->sd_uid\t  = get_be32(&from->sd_uid);\n>> -\tto->sd_gid\t  = get_be32(&from->sd_gid);\n>> -\tto->sd_size\t  = get_be32(&from->sd_size);\n>> +\tmemcpy(to, data, sizeof(*to));\n>> +\tto->sd_ctime.sec  = ntohl(to->sd_ctime.sec);\n>> +\tto->sd_ctime.nsec = ntohl(to->sd_ctime.nsec);\n>> +\tto->sd_mtime.sec  = ntohl(to->sd_mtime.sec);\n>> +\tto->sd_mtime.nsec = ntohl(to->sd_mtime.nsec);\n>> +\tto->sd_dev\t  = ntohl(to->sd_dev);\n>> +\tto->sd_ino\t  = ntohl(to->sd_ino);\n>> +\tto->sd_uid\t  = ntohl(to->sd_uid);\n>> +\tto->sd_gid\t  = ntohl(to->sd_gid);\n>> +\tto->sd_size\t  = ntohl(to->sd_size);\n>>   }\n> \n> Hmm. I would have written this to pull the bytes directly out of the\n> array, like:\n> \n>    to->sd_ctime.sec  = get_be32(data); data += 4;\n>    to->sd_ctime.nsec = get_be32(data); data += 4;\n> \n> etc. Or even a helper to do the advancing like:\n> \n>    to->sd_ctime.sec = parse_be32(&data);\n> \n> That reduces assumptions about padding in \"struct stat_data\". But\n> looking more at this code, and reading your comment:\n> \n>> Side note: The OS name is not enough for determining the layout of\n>> struct ondisk_untracked_cache.  Different platforms can have different\n>> int sizes and padding.  Adding the machine type could help, but that\n>> would be a breaking change.  At that point we would be better off\n>> defining a machine-independent format, no?\n> \n> it looks like assumptions about struct layout are pervasive and part of\n> the on-disk format. Yuck. :(\n\nAssuming that there is no padding probably even works for the platforms\nthe code currently supports (basically x86), but I don't know about\nothers.  We'd need to change the writing side as well to match, though.\nWhich is probably a good idea, but I tried to keep the patch small and\nits impact low.  Cross-machine usability is currently explicitly not\nsupported -- not sure why, though.\n\nRené\n"},{"id":"324628","messageId":"20170716160313.2crnw6exyqpvp7ue@sigill.intra.peff.net","threadId":"46399","inReplyTo":"84b4d5aa-956f-932b-4e95-f181243215bc@web.de","subject":"Re: [PATCH] dir: support platforms that require aligned reads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-16T16:03:13Z","receivedAt":"2017-07-16T16:03:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 16, 2017 at 05:18:27PM +0200, René Scharfe wrote:\n\n> > it looks like assumptions about struct layout are pervasive and part of\n> > the on-disk format. Yuck. :(\n> \n> Assuming that there is no padding probably even works for the platforms\n> the code currently supports (basically x86), but I don't know about\n> others.  We'd need to change the writing side as well to match, though.\n> Which is probably a good idea, but I tried to keep the patch small and\n> its impact low.  Cross-machine usability is currently explicitly not\n> supported -- not sure why, though.\n\nYeah, I think your patch is a good first step, and we don't have to go\nfurther for now.\n\n-Peff\n"}]}