From: Ramsay Jones <ramsay@ramsayjones•plus.com>
To: Jeff King <peff@peff•net>, git@vger•kernel.org
Subject: Re: [PATCH 08/67] add reentrant variants of sha1_to_hex and find_unique_abbrev
Date: Tue, 15 Sep 2015 17:55:55 +0100 [thread overview]
Message-ID: <55F84D9B.90004@ramsayjones.plus.com> (raw)
In-Reply-To: <20150915152629.GH29753@sigill.intra.peff.net>
On 15/09/15 16:26, Jeff King wrote:
> The sha1_to_hex and find_unique_abbrev functions always
> write into reusable static buffers. There are a few problems
> with this:
>
> - future calls overwrite our result. This is especially
> annoying with find_unique_abbrev, which does not have a
> ring of buffers, so you cannot even printf() a result
> that has two abbreviated sha1s.
>
> - if you want to put the result into another buffer, we
> often strcpy, which looks suspicious when auditing for
> overflows.
>
> This patch introduces sha1_to_hex_to and find_unique_abbrev_to,
> which write into a user-provided buffer. Of course this is
> just punting on the overflow-auditing, as the buffer
> obviously needs to be GIT_SHA1_HEXSZ + 1 bytes. But it is
> much easier to audit, since that is a well-known size.
Hmm, I haven't read any other patches yet (including those which use these
new '_to' functions), but I can't help feeling they should be named something
like 'sha1_to_hex_str()' and 'find_unique_abbrev_str()' instead. i.e. I don't get
the '_to' thing - not that I'm any good at naming things ...
ATB,
Ramsay Jones
>
> We retain the non-reentrant forms, which just become thin
> wrappers around the reentrant ones. This patch also adds a
> strbuf variant of find_unique_abbrev, which will be handy in
> later patches.
>
> Signed-off-by: Jeff King <peff@peff•net>
> ---
> If we wanted to be really meticulous, these functions could
> take a size for the output buffer, and complain if it is not
> GIT_SHA1_HEXSZ+1 bytes. But that would bloat every call
> like:
>
> sha1_to_hex_to(buf, sizeof(buf), sha1);
>
> cache.h | 27 ++++++++++++++++++++++++++-
> hex.c | 13 +++++++++----
> sha1_name.c | 16 +++++++++++-----
> strbuf.c | 9 +++++++++
> strbuf.h | 8 ++++++++
> 5 files changed, 63 insertions(+), 10 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index e231e47..cc59aba 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -785,7 +785,21 @@ extern char *sha1_pack_name(const unsigned char *sha1);
> */
> extern char *sha1_pack_index_name(const unsigned char *sha1);
>
> -extern const char *find_unique_abbrev(const unsigned char *sha1, int);
> +/*
> + * Return an abbreviated sha1 unique within this repository's object database.
> + * The result will be at least `len` characters long, and will be NUL
> + * terminated.
> + *
> + * The non-`_to` version returns a static buffer which will be overwritten by
> + * subsequent calls.
> + *
> + * The `_to` variant writes to a buffer supplied by the caller, which must be
> + * at least `GIT_SHA1_HEXSZ + 1` bytes. The return value is the number of bytes
> + * written (excluding the NUL terminator).
> + */
> +extern const char *find_unique_abbrev(const unsigned char *sha1, int len);
> +extern int find_unique_abbrev_to(char *hex, const unsigned char *sha1, int len);
> +
> extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];
>
> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
> @@ -1067,6 +1081,17 @@ extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);
> extern int get_sha1_hex(const char *hex, unsigned char *sha1);
> extern int get_oid_hex(const char *hex, struct object_id *sha1);
>
> +/*
> + * Convert a binary sha1 to its hex equivalent. The `_to` variant writes
> + * the NUL-terminated output to the buffer `out`, which must be at least
> + * `GIT_SHA1_HEXSZ + 1` bytes, and returns a pointer to out for convenience.
> + *
> + * The non-`_to` variant returns a static buffer, but uses a ring of 4
> + * buffers, making it safe to make multiple calls for a single statement, like:
> + *
> + * printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
> + */
> +extern char *sha1_to_hex_to(char *out, const unsigned char *sha1);
> extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
> extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as sha1_to_hex */
>
> diff --git a/hex.c b/hex.c
> index 899b74a..004fdea 100644
> --- a/hex.c
> +++ b/hex.c
> @@ -61,12 +61,10 @@ int get_oid_hex(const char *hex, struct object_id *oid)
> return get_sha1_hex(hex, oid->hash);
> }
>
> -char *sha1_to_hex(const unsigned char *sha1)
> +char *sha1_to_hex_to(char *buffer, const unsigned char *sha1)
> {
> - static int bufno;
> - static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
> static const char hex[] = "0123456789abcdef";
> - char *buffer = hexbuffer[3 & ++bufno], *buf = buffer;
> + char *buf = buffer;
> int i;
>
> for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
> @@ -79,6 +77,13 @@ char *sha1_to_hex(const unsigned char *sha1)
> return buffer;
> }
>
> +char *sha1_to_hex(const unsigned char *sha1)
> +{
> + static int bufno;
> + static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
> + return sha1_to_hex_to(hexbuffer[3 & ++bufno], sha1);
> +}
> +
> char *oid_to_hex(const struct object_id *oid)
> {
> return sha1_to_hex(oid->hash);
> diff --git a/sha1_name.c b/sha1_name.c
> index da6874c..416e408 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -368,14 +368,13 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
> return ds.ambiguous;
> }
>
> -const char *find_unique_abbrev(const unsigned char *sha1, int len)
> +int find_unique_abbrev_to(char *hex, const unsigned char *sha1, int len)
> {
> int status, exists;
> - static char hex[41];
>
> - memcpy(hex, sha1_to_hex(sha1), 40);
> + sha1_to_hex_to(hex, sha1);
> if (len == 40 || !len)
> - return hex;
> + return 40;
> exists = has_sha1_file(sha1);
> while (len < 40) {
> unsigned char sha1_ret[20];
> @@ -384,10 +383,17 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
> ? !status
> : status == SHORT_NAME_NOT_FOUND) {
> hex[len] = 0;
> - return hex;
> + return len;
> }
> len++;
> }
> + return len;
> +}
> +
> +const char *find_unique_abbrev(const unsigned char *sha1, int len)
> +{
> + static char hex[GIT_SHA1_HEXSZ + 1];
> + find_unique_abbrev_to(hex, sha1, len);
> return hex;
> }
>
> diff --git a/strbuf.c b/strbuf.c
> index 29df55b..6c1b577 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -743,3 +743,12 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
> }
> strbuf_setlen(sb, sb->len + len);
> }
> +
> +void strbuf_add_unique_abbrev(struct strbuf *sb, const unsigned char *sha1,
> + int abbrev_len)
> +{
> + int r;
> + strbuf_grow(sb, GIT_SHA1_HEXSZ + 1);
> + r = find_unique_abbrev_to(sb->buf + sb->len, sha1, abbrev_len);
> + strbuf_setlen(sb, sb->len + r);
> +}
> diff --git a/strbuf.h b/strbuf.h
> index ba099cd..9aace36 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -475,6 +475,14 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
> extern void strbuf_list_free(struct strbuf **);
>
> /**
> + * Add the abbreviation, as generated by find_unique_abbrev, of `sha1` to
> + * the strbuf `sb`.
> + */
> +extern void strbuf_add_unique_abbrev(struct strbuf *sb,
> + const unsigned char *sha1,
> + int abbrev_len);
> +
> +/**
> * Launch the user preferred editor to edit a file and fill the buffer
> * with the file's contents upon the user completing their editing. The
> * third argument can be used to set the environment which the editor is
next prev parent reply other threads:[~2015-09-15 16:56 UTC|newest]
Thread overview: 154+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-15 15:21 [PATCH 0/67] war on sprintf, strcpy, etc Jeff King
2015-09-15 15:23 ` [PATCH 01/67] show-branch: avoid segfault with --reflog of unborn branch Jeff King
2015-09-15 15:23 ` [PATCH 02/67] mailsplit: fix FILE* leak in split_maildir Jeff King
2015-09-15 15:23 ` [PATCH 03/67] archive-tar: fix minor indentation violation Jeff King
2015-09-15 15:24 ` [PATCH 04/67] fsck: don't fsck alternates for connectivity-only check Jeff King
2015-09-15 17:55 ` Johannes Schindelin
2015-09-16 18:04 ` Junio C Hamano
2015-09-16 18:12 ` Jeff King
2015-09-16 19:12 ` Junio C Hamano
2015-09-16 19:14 ` Eric Sunshine
2015-09-16 20:00 ` Jeff King
2015-09-15 15:24 ` [PATCH 05/67] add xsnprintf helper function Jeff King
2015-09-15 15:25 ` [PATCH 06/67] add git_path_buf " Jeff King
2015-09-15 15:25 ` [PATCH 07/67] strbuf: make strbuf_complete_line more generic Jeff King
2015-09-16 0:45 ` Eric Sunshine
2015-09-16 1:27 ` Junio C Hamano
2015-09-16 9:57 ` Jeff King
2015-09-16 15:11 ` Eric Sunshine
2015-09-15 15:26 ` [PATCH 08/67] add reentrant variants of sha1_to_hex and find_unique_abbrev Jeff King
2015-09-15 16:55 ` Ramsay Jones [this message]
2015-09-15 17:50 ` Jeff King
2015-09-16 1:32 ` Junio C Hamano
2015-09-16 8:15 ` Johannes Schindelin
2015-09-16 10:33 ` Jeff King
2015-09-16 17:06 ` Junio C Hamano
2015-09-16 17:23 ` Jeff King
2015-09-15 15:26 ` [PATCH 09/67] fsck: use strbuf to generate alternate directories Jeff King
2015-09-15 15:28 ` [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic Jeff King
2015-09-16 0:51 ` Eric Sunshine
2015-09-16 10:14 ` Jeff King
2015-09-16 10:25 ` Jeff King
2015-09-16 18:13 ` Junio C Hamano
2015-09-16 20:22 ` Jeff King
2015-09-15 15:28 ` [PATCH 11/67] trace: use strbuf for quote_crnl output Jeff King
2015-09-16 0:55 ` Eric Sunshine
2015-09-16 10:31 ` Jeff King
2015-09-16 15:16 ` Eric Sunshine
2015-09-15 15:29 ` [PATCH 12/67] progress: store throughput display in a strbuf Jeff King
2015-09-15 15:30 ` [PATCH 13/67] test-dump-cache-tree: avoid overflow of cache-tree name Jeff King
2015-09-15 15:31 ` [PATCH 14/67] compat/inet_ntop: fix off-by-one in inet_ntop4 Jeff King
2015-09-15 15:36 ` [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf Jeff King
2015-09-15 18:32 ` Ramsay Jones
2015-09-15 18:42 ` Jeff King
2015-09-15 19:15 ` Ramsay Jones
2015-09-15 20:38 ` Stefan Beller
2015-09-16 9:45 ` Jeff King
2015-09-16 18:20 ` Junio C Hamano
2015-09-16 1:34 ` Junio C Hamano
2015-09-16 3:19 ` Eric Sunshine
2015-09-16 9:48 ` Jeff King
2015-09-16 18:24 ` Junio C Hamano
2015-09-16 18:52 ` Jeff King
2015-09-16 19:07 ` Junio C Hamano
2015-09-16 19:19 ` Stefan Beller
2015-09-16 20:35 ` Jeff King
2015-09-16 20:32 ` Jeff King
2015-09-15 15:37 ` [PATCH 16/67] archive-tar: use xsnprintf for trivial formatting Jeff King
2015-09-15 15:38 ` [PATCH 17/67] use xsnprintf for generating git object headers Jeff King
2015-09-16 18:30 ` Junio C Hamano
2015-09-15 15:38 ` [PATCH 18/67] find_short_object_filename: convert sprintf to xsnprintf Jeff King
2015-09-15 15:39 ` [PATCH 19/67] stop_progress_msg: " Jeff King
2015-09-15 15:39 ` [PATCH 20/67] compat/hstrerror: convert sprintf to snprintf Jeff King
2015-09-15 15:39 ` [PATCH 21/67] grep: use xsnprintf to format failure message Jeff King
2015-09-15 15:40 ` [PATCH 22/67] entry.c: convert strcpy to xsnprintf Jeff King
2015-09-15 19:01 ` Ramsay Jones
2015-09-15 21:04 ` Stefan Beller
2015-09-15 15:41 ` [PATCH 23/67] add_packed_git: convert strcpy into xsnprintf Jeff King
2015-09-16 18:43 ` Junio C Hamano
2015-09-16 20:24 ` Jeff King
2015-09-15 15:42 ` [PATCH 24/67] http-push: replace strcat with xsnprintf Jeff King
2015-09-15 15:43 ` [PATCH 25/67] receive-pack: convert strncpy to xsnprintf Jeff King
2015-09-15 15:45 ` [PATCH 26/67] replace trivial malloc + sprintf /strcpy calls to xstrfmt Jeff King
2015-09-16 4:24 ` Eric Sunshine
2015-09-16 10:43 ` Jeff King
2015-09-15 15:45 ` [PATCH 27/67] config: use xstrfmt in normalize_value Jeff King
2015-09-15 15:46 ` [PATCH 28/67] fetch: replace static buffer with xstrfmt Jeff King
2015-09-15 15:47 ` [PATCH 29/67] use strip_suffix and xstrfmt to replace suffix Jeff King
2015-09-16 4:38 ` Eric Sunshine
2015-09-16 10:50 ` Jeff King
2015-09-16 15:20 ` Eric Sunshine
2015-09-15 15:48 ` [PATCH 30/67] ref-filter: drop sprintf and strcpy calls Jeff King
2015-09-16 19:33 ` Junio C Hamano
2015-09-15 15:48 ` [PATCH 31/67] help: drop prepend function in favor of xstrfmt Jeff King
2015-09-15 15:49 ` [PATCH 32/67] mailmap: replace strcpy with xstrdup Jeff King
2015-09-15 15:49 ` [PATCH 33/67] read_branches_file: " Jeff King
2015-09-16 19:52 ` Junio C Hamano
2015-09-16 20:42 ` Jeff King
2015-09-17 11:28 ` Jeff King
2015-09-17 11:32 ` Jeff King
2015-09-17 11:36 ` Jeff King
2015-09-17 15:38 ` Junio C Hamano
2015-09-17 16:24 ` Jeff King
2015-09-17 16:53 ` Junio C Hamano
2015-09-15 15:50 ` [PATCH 34/67] resolve_ref: use strbufs for internal buffers Jeff King
2015-09-15 15:51 ` [PATCH 35/67] upload-archive: convert sprintf to strbuf Jeff King
2015-09-15 15:52 ` [PATCH 36/67] remote-ext: simplify git pkt-line generation Jeff King
2015-09-16 20:18 ` Junio C Hamano
2015-09-16 21:23 ` Jeff King
2015-09-15 15:52 ` [PATCH 37/67] http-push: use strbuf instead of fwrite_buffer Jeff King
2015-09-15 15:53 ` [PATCH 38/67] http-walker: store url in a strbuf Jeff King
2015-09-15 15:54 ` [PATCH 39/67] sha1_get_pack_name: use " Jeff King
2015-09-15 15:56 ` [PATCH 40/67] init: use strbufs to store paths Jeff King
2015-09-15 15:57 ` [PATCH 41/67] apply: convert root string to strbuf Jeff King
2015-09-15 15:57 ` [PATCH 42/67] transport: use strbufs for status table "quickref" strings Jeff King
2015-09-15 15:58 ` [PATCH 43/67] merge-recursive: convert malloc / strcpy to strbuf Jeff King
2015-09-15 15:59 ` [PATCH 44/67] enter_repo: convert fixed-size buffers to strbufs Jeff King
2015-09-15 15:59 ` [PATCH 45/67] remove_leading_path: use a strbuf for internal storage Jeff King
2015-09-15 16:00 ` [PATCH 46/67] write_loose_object: convert to strbuf Jeff King
2015-09-16 21:27 ` Junio C Hamano
2015-09-16 21:39 ` Jeff King
2015-09-15 16:01 ` [PATCH 47/67] diagnose_invalid_index_path: use strbuf to avoid strcpy/strcat Jeff King
2015-09-15 16:02 ` [PATCH 48/67] fetch-pack: use argv_array for index-pack / unpack-objects Jeff King
2015-09-15 16:02 ` [PATCH 49/67] http-push: use an argv_array for setup_revisions Jeff King
2015-09-15 16:03 ` [PATCH 50/67] stat_tracking_info: convert to argv_array Jeff King
2015-09-15 16:04 ` [PATCH 51/67] daemon: use cld->env_array when re-spawning Jeff King
2015-09-15 16:05 ` [PATCH 52/67] use sha1_to_hex_to() instead of strcpy Jeff King
2015-09-16 21:51 ` Junio C Hamano
2015-09-16 21:54 ` Jeff King
2015-09-16 21:59 ` Junio C Hamano
2015-09-15 16:06 ` [PATCH 53/67] drop strcpy in favor of raw sha1_to_hex Jeff King
2015-09-18 19:24 ` Eric Sunshine
2015-09-18 19:29 ` Jeff King
2015-09-15 16:07 ` [PATCH 54/67] color: add overflow checks for parsing colors Jeff King
2015-09-18 18:54 ` Eric Sunshine
2015-09-18 19:01 ` Jeff King
2015-09-21 16:56 ` Junio C Hamano
2015-09-15 16:07 ` [PATCH 55/67] use alloc_ref rather than hand-allocating "struct ref" Jeff King
2015-09-15 16:09 ` [PATCH 56/67] avoid sprintf and strcpy with flex arrays Jeff King
2015-09-20 22:48 ` Eric Sunshine
2015-09-21 15:15 ` Jeff King
2015-09-21 17:11 ` Eric Sunshine
2015-09-21 17:19 ` Jeff King
2015-09-15 16:10 ` [PATCH 57/67] receive-pack: simplify keep_arg computation Jeff King
2015-09-18 18:43 ` Eric Sunshine
2015-09-18 18:49 ` Jeff King
2015-09-15 16:11 ` [PATCH 58/67] help: clean up kfmclient munging Jeff King
2015-09-15 16:11 ` [PATCH 59/67] prefer memcpy to strcpy Jeff King
2015-09-15 16:12 ` [PATCH 60/67] color: add color_set helper for copying raw colors Jeff King
2015-09-15 16:13 ` [PATCH 61/67] notes: document length of fanout path with a constant Jeff King
2015-09-15 16:13 ` [PATCH 62/67] convert strncpy to memcpy Jeff King
2015-09-15 16:14 ` [PATCH 63/67] fsck: drop inode-sorting code Jeff King
2015-09-15 16:14 ` [PATCH 64/67] Makefile: drop D_INO_IN_DIRENT build knob Jeff King
2015-09-15 16:15 ` [PATCH 65/67] fsck: use for_each_loose_file_in_objdir Jeff King
2015-09-15 16:16 ` [PATCH 66/67] use strbuf_complete to conditionally append slash Jeff King
2015-09-16 22:18 ` Junio C Hamano
2015-09-16 22:39 ` Jeff King
2015-09-16 22:54 ` Junio C Hamano
2015-09-16 22:57 ` Jeff King
2015-09-17 15:45 ` Junio C Hamano
2015-09-21 1:50 ` Eric Sunshine
2015-09-21 15:17 ` Jeff King
2015-09-15 16:16 ` [PATCH 67/67] name-rev: use strip_suffix to avoid magic numbers Jeff King
2015-09-16 1:54 ` [PATCH 0/67] war on sprintf, strcpy, etc Junio C Hamano
2015-09-16 10:35 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55F84D9B.90004@ramsayjones.plus.com \
--to=ramsay@ramsayjones$(echo .)plus.com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox