Jeff King <p...@peff.net> writes:

> We generally use 32-byte buffers to format git's "type size"
> header fields. These should not generally overflow unless
> you can produce some truly gigantic objects (and our types
> come from our internal array of constant strings). But it is
> a good idea to use xsnprintf to make sure this is the case.
>
> Note that we slightly modify the interface to
> write_sha1_file_prepare, which nows uses "hdrlen" as an "in"
> parameter as well as an "out" (on the way in it stores the
> allocated size of the header, and on the way out it returns
> the ultimate size of the header).

;-).  I skipped this paragraph and jumped directly into the code,
noticed that you did something funny in write-sha1-file-prepare and
did the right thing in hash-sha1-file to compensate for it, and then
came back here to find it properly described.  Now I read the patch
again and I see the other caller is also properly adjusted.

I think this change makes sense.  Thanks.

>
> Signed-off-by: Jeff King <p...@peff.net>
> ---
>  builtin/index-pack.c |  2 +-
>  bulk-checkin.c       |  4 ++--
>  fast-import.c        |  4 ++--
>  http-push.c          |  2 +-
>  sha1_file.c          | 13 +++++++------
>  5 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 3431de2..1ad1bde 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -441,7 +441,7 @@ static void *unpack_entry_data(unsigned long offset, 
> unsigned long size,
>       int hdrlen;
>  
>       if (!is_delta_type(type)) {
> -             hdrlen = sprintf(hdr, "%s %lu", typename(type), size) + 1;
> +             hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type), 
> size) + 1;
>               git_SHA1_Init(&c);
>               git_SHA1_Update(&c, hdr, hdrlen);
>       } else
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 7cffc3a..4347f5c 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -200,8 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state 
> *state,
>       if (seekback == (off_t) -1)
>               return error("cannot find the current offset");
>  
> -     header_len = sprintf((char *)obuf, "%s %" PRIuMAX,
> -                          typename(type), (uintmax_t)size) + 1;
> +     header_len = xsnprintf((char *)obuf, sizeof(obuf), "%s %" PRIuMAX,
> +                            typename(type), (uintmax_t)size) + 1;
>       git_SHA1_Init(&ctx);
>       git_SHA1_Update(&ctx, obuf, header_len);
>  
> diff --git a/fast-import.c b/fast-import.c
> index 6c7c3c9..d0c2502 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1035,8 +1035,8 @@ static int store_object(
>       git_SHA_CTX c;
>       git_zstream s;
>  
> -     hdrlen = sprintf((char *)hdr,"%s %lu", typename(type),
> -             (unsigned long)dat->len) + 1;
> +     hdrlen = xsnprintf((char *)hdr, sizeof(hdr), "%s %lu",
> +                        typename(type), (unsigned long)dat->len) + 1;
>       git_SHA1_Init(&c);
>       git_SHA1_Update(&c, hdr, hdrlen);
>       git_SHA1_Update(&c, dat->buf, dat->len);
> diff --git a/http-push.c b/http-push.c
> index 154e67b..1f3788f 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -361,7 +361,7 @@ static void start_put(struct transfer_request *request)
>       git_zstream stream;
>  
>       unpacked = read_sha1_file(request->obj->sha1, &type, &len);
> -     hdrlen = sprintf(hdr, "%s %lu", typename(type), len) + 1;
> +     hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type), len) + 1;
>  
>       /* Set it up */
>       git_deflate_init(&stream, zlib_compression_level);
> diff --git a/sha1_file.c b/sha1_file.c
> index d295a32..f106091 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1464,7 +1464,7 @@ int check_sha1_signature(const unsigned char *sha1, 
> void *map,
>               return -1;
>  
>       /* Generate the header */
> -     hdrlen = sprintf(hdr, "%s %lu", typename(obj_type), size) + 1;
> +     hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(obj_type), 
> size) + 1;
>  
>       /* Sha1.. */
>       git_SHA1_Init(&c);
> @@ -2930,7 +2930,7 @@ static void write_sha1_file_prepare(const void *buf, 
> unsigned long len,
>       git_SHA_CTX c;
>  
>       /* Generate the header */
> -     *hdrlen = sprintf(hdr, "%s %lu", type, len)+1;
> +     *hdrlen = xsnprintf(hdr, *hdrlen, "%s %lu", type, len)+1;
>  
>       /* Sha1.. */
>       git_SHA1_Init(&c);
> @@ -2993,7 +2993,7 @@ int hash_sha1_file(const void *buf, unsigned long len, 
> const char *type,
>                     unsigned char *sha1)
>  {
>       char hdr[32];
> -     int hdrlen;
> +     int hdrlen = sizeof(hdr);
>       write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
>       return 0;
>  }
> @@ -3139,7 +3139,7 @@ static int freshen_packed_object(const unsigned char 
> *sha1)
>  int write_sha1_file(const void *buf, unsigned long len, const char *type, 
> unsigned char *sha1)
>  {
>       char hdr[32];
> -     int hdrlen;
> +     int hdrlen = sizeof(hdr);
>  
>       /* Normally if we have it in the pack then we do not bother writing
>        * it out into .git/objects/??/?{38} file.
> @@ -3157,7 +3157,8 @@ int hash_sha1_file_literally(const void *buf, unsigned 
> long len, const char *typ
>       int hdrlen, status = 0;
>  
>       /* type string, SP, %lu of the length plus NUL must fit this */
> -     header = xmalloc(strlen(type) + 32);
> +     hdrlen = strlen(type) + 32;
> +     header = xmalloc(hdrlen);
>       write_sha1_file_prepare(buf, len, type, sha1, header, &hdrlen);
>  
>       if (!(flags & HASH_WRITE_OBJECT))
> @@ -3185,7 +3186,7 @@ int force_object_loose(const unsigned char *sha1, 
> time_t mtime)
>       buf = read_packed_sha1(sha1, &type, &len);
>       if (!buf)
>               return error("cannot read sha1_file for %s", sha1_to_hex(sha1));
> -     hdrlen = sprintf(hdr, "%s %lu", typename(type), len) + 1;
> +     hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type), len) + 1;
>       ret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime);
>       free(buf);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to