Taylor Blau <me@xxxxxxxxxxxx> writes:
> diff --git a/pack-mtimes.c b/pack-mtimes.c
> index 46ad584af1..0e0aafdcb0 100644
> --- a/pack-mtimes.c
> +++ b/pack-mtimes.c
> @@ -1,3 +1,4 @@
> +#include "git-compat-util.h"
> #include "pack-mtimes.h"
> #include "object-store.h"
> #include "packfile.h"
> @@ -7,12 +8,10 @@ static char *pack_mtimes_filename(struct packed_git *p)
> size_t len;
> if (!strip_suffix(p->pack_name, ".pack", &len))
> BUG("pack_name does not end in .pack");
> - /* NEEDSWORK: this could reuse code from pack-revindex.c. */
> return xstrfmt("%.*s.mtimes", (int)len, p->pack_name);
> }
>
> #define MTIMES_HEADER_SIZE (12)
> -#define MTIMES_MIN_SIZE (MTIMES_HEADER_SIZE + (2 * the_hash_algo->rawsz))
>
> struct mtimes_header {
> uint32_t signature;
> @@ -26,10 +25,9 @@ static int load_pack_mtimes_file(char *mtimes_file,
> {
> int fd, ret = 0;
> struct stat st;
> - void *data = NULL;
> - size_t mtimes_size;
> + uint32_t *data = NULL;
> + size_t mtimes_size, expected_size;
> struct mtimes_header header;
> - uint32_t *hdr;
>
> fd = git_open(mtimes_file);
>
> @@ -44,21 +42,16 @@ static int load_pack_mtimes_file(char *mtimes_file,
>
> mtimes_size = xsize_t(st.st_size);
>
> - if (mtimes_size < MTIMES_MIN_SIZE) {
> + if (mtimes_size < MTIMES_HEADER_SIZE) {
> ret = error(_("mtimes file %s is too small"), mtimes_file);
> goto cleanup;
> }
>
> - if (mtimes_size - MTIMES_MIN_SIZE != st_mult(sizeof(uint32_t), num_objects)) {
> - ret = error(_("mtimes file %s is corrupt"), mtimes_file);
> - goto cleanup;
> - }
> + data = xmmap(NULL, mtimes_size, PROT_READ, MAP_PRIVATE, fd, 0);
>
> - data = hdr = xmmap(NULL, mtimes_size, PROT_READ, MAP_PRIVATE, fd, 0);
> -
> - header.signature = ntohl(hdr[0]);
> - header.version = ntohl(hdr[1]);
> - header.hash_id = ntohl(hdr[2]);
> + header.signature = ntohl(data[0]);
> + header.version = ntohl(data[1]);
> + header.hash_id = ntohl(data[2]);
So, instead of assuming that the size of the file is cast in stone
to match the size the current implementation happens to give and
reject a file from a future version, we check the header first to
give a more readable error when we see a version of the file that
we do not understand.
Makes sense.
At least, "here is a small fixup!" should have been accompanied by a
brief explanation to say something like that, i.e. why a fixup is
needed, what shortcoming in the original it is meant to address,
etc.
Will queue between 2/17 and 3/17 without squashing (yet).
Thanks.
> if (header.signature != MTIMES_SIGNATURE) {
> ret = error(_("mtimes file %s has unknown signature"), mtimes_file);
> @@ -77,13 +70,23 @@ static int load_pack_mtimes_file(char *mtimes_file,
> goto cleanup;
> }
>
> +
> + expected_size = MTIMES_HEADER_SIZE;
> + expected_size = st_add(expected_size, st_mult(sizeof(uint32_t), num_objects));
> + expected_size = st_add(expected_size, 2 * (header.hash_id == 1 ? GIT_SHA1_RAWSZ : GIT_SHA256_RAWSZ));
> +
> + if (mtimes_size != expected_size) {
> + ret = error(_("mtimes file %s is corrupt"), mtimes_file);
> + goto cleanup;
> + }
> +
> cleanup:
> if (ret) {
> if (data)
> munmap(data, mtimes_size);
> } else {
> *len_p = mtimes_size;
> - *data_p = (const uint32_t *)data;
> + *data_p = data;
> }
>
> close(fd);
> diff --git a/pack-mtimes.h b/pack-mtimes.h
> index 38ddb9f893..cc957b3e85 100644
> --- a/pack-mtimes.h
> +++ b/pack-mtimes.h
> @@ -8,8 +8,19 @@
>
> struct packed_git;
>
> +/*
> + * Loads the .mtimes file corresponding to "p", if any, returning zero
> + * on success.
> + */
> int load_pack_mtimes(struct packed_git *p);
>
> +/* Returns the mtime associated with the object at position "pos" (in
> + * lexicographic/index order) in pack "p".
> + *
> + * Note that it is a BUG() to call this function if either (a) "p" does
> + * not have a corresponding .mtimes file, or (b) it does, but it hasn't
> + * been loaded
> + */
> uint32_t nth_packed_mtime(struct packed_git *p, uint32_t pos);
>
> #endif
> --
> 2.36.1.94.gb0d54bedca
>
> --- >8 ---
