From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Michael Haggerty <mhagger@alum•mit.edu>, git@vger•kernel.org
Subject: Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer
Date: Wed, 18 Dec 2013 11:35:34 -0800 [thread overview]
Message-ID: <xmqqa9fyhrzt.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20131217232231.GA14807@sigill.intra.peff.net> (Jeff King's message of "Tue, 17 Dec 2013 18:22:31 -0500")
Jeff King <peff@peff•net> writes:
> Converting it to use strbuf looks like it will actually let us drop a
> bunch of copying, too, as we just end up in mkpath at the very lowest
> level. I.e., something like below.
Thanks; I may have a few minor comments, but overall, I like the
placement of mkpath() in the resulting callchain a lot better than
the original.
> As an aside, I have noticed us using this "push/pop" approach to treating a
> strbuf as a stack of paths elsewhere, too. I.e:
>
> size_t baselen = base->len;
> strbuf_addf(base, "/%s", some_thing);
> some_call(base);
> base->len = baselen;
>
> I wondered if there was any kind of helper we could add to make it look
> nicer. But I don't think so; the hairy part is that you must remember to
> reset base->len after the call, and there is no easy way around that in
> C. If you had object destructors that ran as the stack unwound, or
> dynamic scoping, it would be easy to manipulate the object. Wrapping
> won't work because strbuf isn't just a length wrapping an immutable
> buffer; it actually has to move the NUL in the buffer.
>
> Anyway, not important, but perhaps somebody is more clever than I am.
Hmph... interesting but we would need a lot more thought than the
time necessary to respond to one piece of e-mail for this ;-)
Perhaps later...
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 6366917..4ca8ec1 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -17,9 +17,8 @@ static int verbose;
> static unsigned long expire;
> static int show_progress = -1;
>
> -static int prune_tmp_object(const char *path, const char *filename)
> +static int prune_tmp_object(const char *fullpath)
> {
> - const char *fullpath = mkpath("%s/%s", path, filename);
> struct stat st;
> if (lstat(fullpath, &st))
> return error("Could not stat '%s'", fullpath);
This function is called to remove
* Any tmp_* found directly in .git/objects/
* Any tmp_* found directly in .git/objects/pack/
* Any tmp_obj_* found directly in .git/objects/??/
and shares the same expiration logic with prune_object(). The only
difference from the other function is what the file is called in
dry-run or verbose report ("stale temporary file" vs "<sha-1> <typename>").
We may want to rename it to prune_tmp_file(); its usage may have
been limited to an unborn loose object file at some point in the
history, but it does not look that way in today's code.
> -static int prune_dir(int i, char *path)
> +static int prune_dir(int i, struct strbuf *path)
> {
> - DIR *dir = opendir(path);
> + size_t baselen = path->len;
> + DIR *dir = opendir(path->buf);
> struct dirent *de;
>
> if (!dir)
> @@ -77,28 +76,39 @@ static int prune_dir(int i, char *path)
> if (lookup_object(sha1))
> continue;
>
> - prune_object(path, de->d_name, sha1);
> + strbuf_addf(path, "/%s", de->d_name);
> + prune_object(path->buf, sha1);
> + path->len = baselen;
This is minor, but I prefer using strbuf_setlen() for this.
Thanks.
next prev parent reply other threads:[~2013-12-18 19:35 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-17 13:43 [PATCH 0/3] Fix two buffer overflows and remove a redundant var Michael Haggerty
2013-12-17 13:43 ` [PATCH 1/3] prune-packed: fix a possible buffer overflow Michael Haggerty
2013-12-17 13:57 ` Duy Nguyen
2013-12-17 18:43 ` Junio C Hamano
2013-12-18 22:44 ` Michael Haggerty
2013-12-19 0:04 ` Jeff King
2013-12-19 16:33 ` Michael Haggerty
2013-12-20 9:30 ` Jeff King
2013-12-19 0:37 ` Duy Nguyen
2013-12-17 13:43 ` [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer Michael Haggerty
2013-12-17 18:51 ` Junio C Hamano
2013-12-17 23:22 ` Jeff King
2013-12-18 19:35 ` Junio C Hamano [this message]
2013-12-18 20:00 ` Jeff King
2013-12-18 20:07 ` Junio C Hamano
2013-12-18 20:11 ` Jeff King
2013-12-18 20:15 ` Junio C Hamano
2013-12-18 20:27 ` Jeff King
2013-12-17 18:56 ` Antoine Pelisse
2013-12-17 13:43 ` [PATCH 3/3] cmd_repack(): remove redundant local variable "nr_packs" Michael Haggerty
2013-12-17 13:46 ` Stefan Beller
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=xmqqa9fyhrzt.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=mhagger@alum$(echo .)mit.edu \
--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