Re: [PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 28, 2014, 09:15 UTC
Message-ID: <CAPig+cTXK6=LDPDii6RQyO1fiMCq0Rai5uO0JYOzunM=4c_nUw@mail.gmail.com>
In-Reply-To: <1393574305-24015-1-git-send-email-faiz.off93@gmail.com>

On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari <faiz.off93@gmail.com> wrote:

Show 22 quoted lines

> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>
>
> Notes:
>     I finally got what's happening, and why the errors were caused.
>     packname is supposed to contain the complete path to the .pack file.
>     Packs are stored as /path/to/<SHA1>.pack which I overlooked earlier.
>     After inspecting what is happening in pack-write.c:finish_tmp_packfile()
>     which indirectly modifies packname by appending the SHA1 and ".pack" to packname
>     This is happening in these code snippets:
>         char *end_of_name_prefix = strrchr(name_buffer, 0);
>
>     and later
>         sprintf(end_of_name_prefix, "%s.pack", sha1_to_hex(sha1));
>
>     name_buffer is packname.buf
>     Using const for the first argument of pack-write.c:finish_tmp_packfile()
>     doesnot raise any compile time warning or error and not any runtime errors,
>     since the packname.buf is on heap and has extra space to which more char can be written.
>     If this was not the case,
>         for e.g. passing a constant string and modifying it.
>         This will result in a segmentation fault.
> ---

This notes section is important to the ongoing email discussion, however, it should be placed below the "---" line so that it does not become part of the recorded commit message when the patch is applied via "git am".

Show 24 quoted lines

>  bulk-checkin.c |    8 +++++---
>  pack-write.c   |    2 +-
>  pack.h         |    2 +-
>  3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 118c625..bbdf1ec 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -23,7 +23,7 @@ static struct bulk_checkin_state {
>  static void finish_bulk_checkin(struct bulk_checkin_state *state)
>  {
>         unsigned char sha1[20];
> -       char packname[PATH_MAX];
> +       struct strbuf packname = STRBUF_INIT;
>         int i;
>
>         if (!state->f)
> @@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
>                                          state->offset);
>                 close(fd);
>         }
> +       strbuf_addf(&packname, "%s/pack/pack-", get_object_directory());
> +       strbuf_grow(&packname, 40 + 5);

There are several problems with this. First, magic numbers 40 and 5 convey no meaning to the reader. At the very least, they should be named constants or a comment should explain them. More seriously, though, this code is fragile since it has far too intimate knowledge of the inner workings of finish_tmp_packfile(). If the implementation of finish_tmp_packfile() changes in the future such that it writes more than 45 additional characters to the incoming buffer, this will break.

Rather than coupling finish_bulk_checkin() and finish_tmp_packfile() so tightly, consider finish_tmp_packfile() a black box which just "does its job" and then propose ways to make things work without finish_bulk_checkin() having to know how that job is done.

Show 16 quoted lines

> -       sprintf(packname, "%s/pack/pack-", get_object_directory());
> -       finish_tmp_packfile(packname, state->pack_tmp_name,
> +       finish_tmp_packfile(packname.buf, state->pack_tmp_name,
>                             state->written, state->nr_written,
>                             &state->pack_idx_opts, sha1);
>         for (i = 0; i < state->nr_written; i++)
> diff --git a/pack-write.c b/pack-write.c
> index 605d01b..ac38867 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
>         return sha1fd(fd, *pack_tmp_name);
>  }
>
> -void finish_tmp_packfile(char *name_buffer,
> +void finish_tmp_packfile(const char *name_buffer,

This is misleading and fragile. By specifying 'const', finish_tmp_packfile() promises not to modify the content of the incoming name_buffer, yet it breaks this promise by modifying the buffer through the non-const end_of_name_prefix variable (after dropping the 'const' via strrchr()).

>                          const char *pack_tmp_name,
>                          struct pack_idx_entry **written_list,
>                          uint32_t nr_written,