* Re: [PATCH] git-gui: stop using deprecated merge syntax
From: Stefan Beller @ 2016-09-26 17:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, René Scharfe, Git List, Pat Thoyts,
Dennis Kaarsemaker
In-Reply-To: <xmqqvaxjygb2.fsf@gitster.mtv.corp.google.com>
On Sun, Sep 25, 2016 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 24.09.2016 um 13:30 schrieb René Scharfe:
>>> Starting with v2.5.0 git merge can handle FETCH_HEAD internally and
>>> warns when it's called like 'git merge <message> HEAD <commit>' because
>>> that syntax is deprecated. Use this feature in git-gui and get rid of
>>> that warning.
>>>
>>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>>> ---
>>> Tested only _very_ lightly!
>>>
>>> git-gui/lib/merge.tcl | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/git-gui/lib/merge.tcl b/git-gui/lib/merge.tcl
>>> index 460d32f..5ab6f8f 100644
>>> --- a/git-gui/lib/merge.tcl
>>> +++ b/git-gui/lib/merge.tcl
>>> @@ -112,12 +112,7 @@ method _start {} {
>>> close $fh
>>> set _last_merged_branch $branch
>>>
>>> - set cmd [list git]
>>> - lappend cmd merge
>>> - lappend cmd --strategy=recursive
>>> - lappend cmd [git fmt-merge-msg <[gitdir FETCH_HEAD]]
>>> - lappend cmd HEAD
>>> - lappend cmd $name
>>> + set cmd [list git merge --strategy=recursive FETCH_HEAD]
>>>
>>> ui_status [mc "Merging %s and %s..." $current_branch $stitle]
>>> set cons [console::new [mc "Merge"] "merge $stitle"]
>>>
>>
>> Much better than my version. I had left fmt-merge-msg and added
>> --no-log to treat merge.log config suitably. But this works too, and
>> is much more obvious.
>>
>> Tested-by: Johannes Sixt <j6t@kdbg.org>
Reviewed-by: Stefan Beller <sbeller@google.com>
^ permalink raw reply
* Re: [PATCH 04/10] get_short_sha1: peel tags when looking for treeish
From: Jeff King @ 2016-09-26 17:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq7f9yvbwn.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 09:55:20AM -0700, Junio C Hamano wrote:
> > Instead, we can use parse_object(). Note that this is the
> > same fix done by 94d75d1 (get_short_sha1(): correctly
> > disambiguate type-limited abbreviation, 2013-07-01), but
> > that commit fixed only the committish disambiguator, and
> > left the bug in the treeish one.
>
> Can you share your secret tool you use to find this kind of thing?
> Yes, the patch from that commit does look very similar to what we
> see in this patch, but I'd love to see "I am fixing an incorrect
> call to lookup-object by replacing it with parse-object; has there
> been a similar fix?" automated ;-)
I wish there was an answer besides "persistence and patience". I was
just finishing up the commit message for the final patch, and noticed
that the tag was not present in the second example output, which happens
to use the tree-ish syntax. And I noticed it was doubly weird that the
same bug did not show up in the test scripts, which look for
committishes. That made me peek at the implementation, and from there it
was an easy `git blame` away.
-Peff
^ permalink raw reply
* Re: [PATCH 06/10] get_short_sha1: NUL-terminate hex prefix
From: Jeff King @ 2016-09-26 17:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq37kmvb6x.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 10:10:46AM -0700, Junio C Hamano wrote:
> > struct disambiguate_state {
> > int len; /* length of prefix in hex chars */
> > - char hex_pfx[GIT_SHA1_HEXSZ];
> > + char hex_pfx[GIT_SHA1_HEXSZ + 1];
> > unsigned char bin_pfx[GIT_SHA1_RAWSZ];
> >
> > disambiguate_hint_fn fn;
> > @@ -291,7 +291,6 @@ static int init_object_disambiguation(const char *name, int len,
> > return -1;
> >
> > memset(ds, 0, sizeof(*ds));
> > - memset(ds->hex_pfx, 'x', GIT_SHA1_HEXSZ);
>
> As the whole thing is cleared here...
>
> >
> > for (i = 0; i < len ;i++) {
> > unsigned char c = name[i];
> > @@ -313,6 +312,7 @@ static int init_object_disambiguation(const char *name, int len,
> > }
> >
> > ds->len = len;
> > + ds->hex_pfx[len] = '\0';
>
> ... do we even need this one? It would not hurt, though.
Sharp eyes. I noticed that while writing it, but wondered if anybody
else would. :)
I left the second one in to make the intention more explicit, and so
readers did not have to worry that the NULs were overwritten in the
loop. I'd be OK with it either way, though.
-Peff
^ permalink raw reply
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Junio C Hamano @ 2016-09-26 17:30 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926120036.mqs435a36njeihq6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We also restrict the list to those that match any
> disambiguation hint. E.g.:
>
> $ git rev-parse b2e1:foo
> error: short SHA1 b2e1 is ambiguous
> hint: The candidates are:
> hint: b2e1196 tag v2.8.0-rc1
> hint: b2e11d1 tree
> hint: b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options'
> fatal: Invalid object name 'b2e1'.
>
> does not bother reporting the blobs, because they cannot
> work as a treeish.
That's a nice touch, and it even comes free--how wonderful.
It somehow felt strange to have an expensive (compared to no-op,
anyway) loop whose only externally visible effect is to call
advise(), but there does not appear to be a way to even disable this
advise() output, so it probably is OK, I guess.
>
> +test_expect_success C_LOCALE_OUTPUT 'ambiguity hints' '
> + test_must_fail git rev-parse 000000000 2>stderr &&
> + grep ^hint: stderr >hints &&
> + # 16 candidates, plus one intro line
> + test_line_count = 17 hints
> +'
> +
> +test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
> + test_must_fail git rev-parse 000000000^{commit} 2>stderr &&
> + grep ^hint: stderr >hints &&
> + # 5 commits, 1 tag (which is a commitish), plus intro line
> + test_line_count = 7 hints
> +'
> +
> +test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '
> + # these two blobs share the same prefix "ee3d", but neither
> + # will pass for a commit
> + echo 851 | git hash-object --stdin -w &&
> + echo 872 | git hash-object --stdin -w &&
> + test_must_fail git rev-parse ee3d^{commit} 2>stderr &&
> + grep ^hint: stderr >hints &&
> + test_line_count = 3 hints
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH] unpack_sha1_header(): detect malformed object header
From: Junio C Hamano @ 2016-09-26 17:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak, Gustavo Grieco
In-Reply-To: <xmqqfuomvdqe.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Good that your attempt to signature-changing change caught it. I'll
> take a further look.
So here are two patch series. The first one makes sure all callers
of parse_sha1_header() check the returned status.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 26 Sep 2016 09:23:41 -0700
Subject: [PATCH 1/2] streaming: make sure to notice corrupt object
The streaming read interface from a loose object called
parse_sha1_header() but discarded its return value, without noticing
a potential error.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
streaming.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/streaming.c b/streaming.c
index 811fcc2..884a8f1 100644
--- a/streaming.c
+++ b/streaming.c
@@ -347,7 +347,8 @@ static open_method_decl(loose)
return -1;
}
- parse_sha1_header(st->u.loose.hdr, &st->size);
+ if (parse_sha1_header(st->u.loose.hdr, &st->size) < 0)
+ return -1;
st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;
st->u.loose.hdr_avail = st->z.total_out;
st->z_state = z_used;
--
2.10.0-533-ga18d90d
^ permalink raw reply related
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Jeff King @ 2016-09-26 17:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqqwphytvp3.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 10:30:48AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > We also restrict the list to those that match any
> > disambiguation hint. E.g.:
> >
> > $ git rev-parse b2e1:foo
> > error: short SHA1 b2e1 is ambiguous
> > hint: The candidates are:
> > hint: b2e1196 tag v2.8.0-rc1
> > hint: b2e11d1 tree
> > hint: b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options'
> > fatal: Invalid object name 'b2e1'.
> >
> > does not bother reporting the blobs, because they cannot
> > work as a treeish.
>
> That's a nice touch, and it even comes free--how wonderful.
>
> It somehow felt strange to have an expensive (compared to no-op,
> anyway) loop whose only externally visible effect is to call
> advise(), but there does not appear to be a way to even disable this
> advise() output, so it probably is OK, I guess.
Right, advise() always has an effect. But that reminds me. I wasn't
sure if we should attach an advice.* config to this. If we do, then the
right place to put the conditional is right after the error() call in
get_short_sha1().
Since it's attached to an error path, I'm guessing nobody will be too
upset about it, so my inclination was to wait and let somebody add the
conditional advice code if they're bothered.
-Peff
^ permalink raw reply
* Re: [PATCH] unpack_sha1_header(): detect malformed object header
From: Junio C Hamano @ 2016-09-26 17:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak, Gustavo Grieco
In-Reply-To: <xmqqfuomvdqe.fsf@gitster.mtv.corp.google.com>
And the second one, that no longer checks SP in unpacker, looks like
this.
-- >8 --
Subject: [PATCH] unpack_sha1_header(): detect malformed object header
When opening a loose object file, we often do this sequence:
- prepare a short buffer for the object header (on stack)
- call unpack_sha1_header() and have early part of the object data
inflated, enough to fill the buffer
- parse that data in the short buffer, assuming that the first part
of the object is <typename> SP <length> NUL
Because the parsing function parse_sha1_header_extended() is not
given the number of bytes inflated into the header buffer, it you
craft a file whose early part inflates a garbage sequence without SP
or NUL, and replace a loose object with it, it will end up reading
past the end of the inflated data.
To correct this, do the following four things:
- rename unpack_sha1_header() to unpack_sha1_short_header() and
have unpack_sha1_header_to_strbuf() keep calling that as its
helper function. This will detect and report zlib errors, but is
not aware of the format of a loose object (as before).
- introduce unpack_sha1_header() that calls the same helper
function, and when zlib reports it inflated OK into the buffer,
check if the inflated data has NUL. This would ensure that
parsing function will terminate within the buffer that holds the
inflated header.
- update unpack_sha1_header_to_strbuf() to check if the resulting
buffer has NUL for the same effect.
- update parse_sha1_header_extended() to make sure that its loop to
find the SP that terminates the <typename> stops at NUL.
Essentially, this makes unpack_*() functions that are asked to
unpack a loose object header to be a bit more strict and detect an
input that cannot possibly be a valid object header, even before the
parsing function kicks in.
Reported-by: Gustavo Grieco <gustavo.grieco@imag.fr>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
sha1_file.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 17262e1..f7054d3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1566,7 +1566,9 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
return used;
}
-int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)
+static int unpack_sha1_short_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
{
/* Get the data stream */
memset(stream, 0, sizeof(*stream));
@@ -1579,13 +1581,31 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
return git_inflate(stream, 0);
}
+int unpack_sha1_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
+{
+ int status = unpack_sha1_short_header(stream, map, mapsize,
+ buffer, bufsiz);
+
+ if (status < Z_OK)
+ return status;
+
+ /* Make sure we have the terminating NUL */
+ if (!memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
+ return -1;
+ return 0;
+}
+
static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
unsigned long mapsize, void *buffer,
unsigned long bufsiz, struct strbuf *header)
{
int status;
- status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
+ status = unpack_sha1_short_header(stream, map, mapsize, buffer, bufsiz);
+ if (status < Z_OK)
+ return -1;
/*
* Check if entire header is unpacked in the first iteration.
@@ -1676,6 +1696,8 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
*/
for (;;) {
char c = *hdr++;
+ if (!c)
+ return -1;
if (c == ' ')
break;
type_len++;
--
2.10.0-533-ga18d90d
^ permalink raw reply related
* Re: [PATCH] unpack_sha1_header(): detect malformed object header
From: Jeff King @ 2016-09-26 17:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Karthik Nayak, Gustavo Grieco
In-Reply-To: <xmqqshsmtvju.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 10:33:57AM -0700, Junio C Hamano wrote:
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Mon, 26 Sep 2016 09:23:41 -0700
> Subject: [PATCH 1/2] streaming: make sure to notice corrupt object
>
> The streaming read interface from a loose object called
> parse_sha1_header() but discarded its return value, without noticing
> a potential error.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> streaming.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/streaming.c b/streaming.c
> index 811fcc2..884a8f1 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -347,7 +347,8 @@ static open_method_decl(loose)
> return -1;
> }
>
> - parse_sha1_header(st->u.loose.hdr, &st->size);
> + if (parse_sha1_header(st->u.loose.hdr, &st->size) < 0)
> + return -1;
Do you have to git_inflate_end() and munmap() here, as the error path
above does (this was missing from my patch, too)?
-Peff
^ permalink raw reply
* Re: [PATCH 06/10] get_short_sha1: NUL-terminate hex prefix
From: Junio C Hamano @ 2016-09-26 17:36 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926172516.frftagyt6aycp75q@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I left the second one in to make the intention more explicit, and so
> readers did not have to worry that the NULs were overwritten in the
> loop. I'd be OK with it either way, though.
Yes, I agree with that it is a good thing to make our intention more
explicit and I am perfectly fine with leaving it as-is.
Thanks.
^ permalink raw reply
* Re: [PATCH] unpack_sha1_header(): detect malformed object header
From: Jeff King @ 2016-09-26 17:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Karthik Nayak, Gustavo Grieco
In-Reply-To: <xmqqoa3atviv.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 10:34:32AM -0700, Junio C Hamano wrote:
> And the second one, that no longer checks SP in unpacker, looks like
> this.
This looks good from a cursory read (but I am about to go to sleep, so
might be a bit less careful than usual :) ).
-Peff
^ permalink raw reply
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Junio C Hamano @ 2016-09-26 17:39 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926173413.prp3wevf6kkksy7c@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Since it's attached to an error path, I'm guessing nobody will be too
> upset about it, so my inclination was to wait and let somebody add the
> conditional advice code if they're bothered.
Fair enough. At that point of getting an error message, the only
thing they can do is to start wondering what object the person who
gave the now-non-unique abbrevation to them, so I suspect this is
one of the "advice" messages that can always be there.
^ permalink raw reply
* Re: [PATCH] unpack_sha1_header(): detect malformed object header
From: Junio C Hamano @ 2016-09-26 17:39 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak, Gustavo Grieco
In-Reply-To: <20160926173540.j4rxcks3pmdcyvtd@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> diff --git a/streaming.c b/streaming.c
>> index 811fcc2..884a8f1 100644
>> --- a/streaming.c
>> +++ b/streaming.c
>> @@ -347,7 +347,8 @@ static open_method_decl(loose)
>> return -1;
>> }
>>
>> - parse_sha1_header(st->u.loose.hdr, &st->size);
>> + if (parse_sha1_header(st->u.loose.hdr, &st->size) < 0)
>> + return -1;
>
> Do you have to git_inflate_end() and munmap() here, as the error path
> above does (this was missing from my patch, too)?
Ah, definitely. We'd need to be consistent; otherwise we'd be
either leaking resources (or existing one double-freeing).
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Gustavo Grieco @ 2016-09-26 17:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com>
Hello,
Now that the cause of this issue is identified, i would like to know if there is an impact in the security, so i can request a CVE if necessary.
Thanks!
^ permalink raw reply
* Re: [PATCH 01/10] get_sha1: detect buggy calls with multiple disambiguators
From: Junio C Hamano @ 2016-09-26 17:50 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926172111.sve6tsse2figcved@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Other than your reinvention of HAS_MULTI_BITS(), which has been with
>> us since db7244bd ("parse-options new features.", 2007-11-07), this
>> looks like a reasonable thing to do.
>
> Heh, I _thought_ we had something like that but couldn't find it. I
> grepped for "[^&]& .*-", which does match it, but stupidly did it only
> in '*.c'. Definitely it should use the existing macro instead.
OK, I'll queue this on top to be squashed, so no need to resend only
for this one.
sha1_name.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index f9812ff..0ff83a9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -310,11 +310,6 @@ static int prepare_prefixes(const char *name, int len,
return 0;
}
-static int multiple_bits_set(unsigned flags)
-{
- return !!(flags & (flags - 1));
-}
-
static int get_short_sha1(const char *name, int len, unsigned char *sha1,
unsigned flags)
{
@@ -333,7 +328,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
memset(&ds, 0, sizeof(ds));
- if (multiple_bits_set(flags & GET_SHA1_DISAMBIGUATORS))
+ if (HAS_MULTI_BITS(flags & GET_SHA1_DISAMBIGUATORS))
die("BUG: multiple get_short_sha1 disambiguator flags");
if (flags & GET_SHA1_COMMIT)
^ permalink raw reply related
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Junio C Hamano @ 2016-09-26 17:55 UTC (permalink / raw)
To: Gustavo Grieco; +Cc: git
In-Reply-To: <790613313.8353074.1474912139102.JavaMail.zimbra@imag.fr>
Gustavo Grieco <gustavo.grieco@imag.fr> writes:
> Now that the cause of this issue is identified, i would like to
> know if there is an impact in the security, so i can request a CVE
> if necessary.
I am inclined to say that it has no security implications. You have
to be able to write a bogus loose object in an object store you
already have write access to in the first place, in order to cause
this read-only access that goes beyond what is allocated, so at the
worst, what you can do is to hurt yourself, and you can already hurt
yourself in various other ways.
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Gustavo Grieco @ 2016-09-26 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq37kmtukf.fsf@gitster.mtv.corp.google.com>
Fair enough. We are testing our tool to try to find bugs/vulnerabilities in several git implementations. I will report here my results if i can find some other memory issue in this git client.
----- Original Message -----
> Gustavo Grieco <gustavo.grieco@imag.fr> writes:
>
> > Now that the cause of this issue is identified, i would like to
> > know if there is an impact in the security, so i can request a CVE
> > if necessary.
>
> I am inclined to say that it has no security implications. You have
> to be able to write a bogus loose object in an object store you
> already have write access to in the first place, in order to cause
> this read-only access that goes beyond what is allocated, so at the
> worst, what you can do is to hurt yourself, and you can already hurt
> yourself in various other ways.
>
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Junio C Hamano @ 2016-09-26 18:06 UTC (permalink / raw)
To: Gustavo Grieco; +Cc: git
In-Reply-To: <48506427.8357405.1474912907126.JavaMail.zimbra@imag.fr>
Gustavo Grieco <gustavo.grieco@imag.fr> writes:
> Fair enough. We are testing our tool to try to find
> bugs/vulnerabilities in several git implementations. I will report
> here my results if i can find some other memory issue in this git
> client.
Thanks. With or without security implications, it is basic codebase
hygiene to identify and correct these issues, and your help is
highly appreciated.
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Junio C Hamano @ 2016-09-26 18:10 UTC (permalink / raw)
To: Gustavo Grieco; +Cc: git
In-Reply-To: <xmqq37kmtukf.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I am inclined to say that it has no security implications. You have
> to be able to write a bogus loose object in an object store you
> already have write access to in the first place, in order to cause
> this ...
Note that you could social-engineer others to fetch from you and
feed a small enough update that results in loose objects created in
their repositories, without you having a direct write access to the
repository.
The codepath under discussion in this thread however cannot be used
as an attack vector via that route, because the "fetch from
elsewhere" codepath runs verification of the incoming data stream
before storing the results (either in loose object files, or in a
packfile) on disk.
^ permalink raw reply
* Re: [PATCH v2 04/11] i18n: add--interactive: mark plural strings
From: Vasco Almeida @ 2016-09-26 18:15 UTC (permalink / raw)
To: Junio C Hamano, git
Cc: Jiang Xin, Ævar Arnfjörð Bjarmason, David Aguilar
In-Reply-To: <1472646690-9699-5-git-send-email-vascomalmeida@sapo.pt>
A Qua, 31-08-2016 às 12:31 +0000, Vasco Almeida escreveu:
> Mark plural strings for translation. Unfold each action case in one
> entire sentence.
>
> Pass new keyword for xgettext to extract.
>
> Update test to include new subrotine Q__() for plural strings handling.
>
> > Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
> ---
> Makefile | 3 ++-
> git-add--interactive.perl | 24 ++++++++++++++++--------
> perl/Git/I18N.pm | 4 +++-
> t/t0202/test.pl | 11 ++++++++++-
> 4 files changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 4e1e857..08badfa 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -666,12 +666,18 @@ sub status_cmd {
> sub say_n_paths {
> > my $did = shift @_;
> > my $cnt = scalar @_;
> > - print "$did ";
> > - if (1 < $cnt) {
> > - print "$cnt paths\n";
> > - }
> > - else {
> > - print "one path\n";
> > + if ($did eq 'added') {
> > + printf(Q__("added one path\n", "added %d paths\n",
> > + $cnt), $cnt);
> > + } elsif ($did eq 'updated') {
> > + printf(Q__("updated one path\n", "updated %d paths\n",
> > + $cnt), $cnt);
> > + } elsif ($did eq 'reversed') {
> > + printf(Q__("reversed one path\n", "reversed %d paths\n",
This should be 'reverted' not 'reversed'.
> > + $cnt), $cnt);
> > + } else {
> > + printf(Q__("touched one path\n", "touched %d paths\n",
> > + $cnt), $cnt);
> > }
> }
When $cnt is 1 I get the following warning:
Redundant argument in printf at /home/vasco/dev/local/libexec/git-core/git-add--interactive line 680.
The singular form does not have a %d to consume $cnt argument to
printf(). Either we find a way to suppress that warning or we change
the singular form to contain %d.
> @@ -1508,8 +1514,10 @@ sub patch_update_file {
> ...
> > - print colored $header_color, "Split into ",
> > - scalar(@split), " hunks.\n";
> > + print colored $header_color, sprintf(
> > + Q__("Split into %d hunk.\n",
> > + "Split into %d hunks.\n",
> > + scalar(@split)), scalar(@split));
Like we do with this.
> >
> >
^ permalink raw reply
* Re: [PATCH 0/3] recursive support for ls-files
From: Junio C Hamano @ 2016-09-26 18:17 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git
In-Reply-To: <20160926170429.GA3624@google.com>
Brandon Williams <bmwill@google.com> writes:
> In a previous email you mentioned that this feature should be completely
> hidden from users, which is why I removed the command line option for
> this latest series.
I may have said something like that; I do not recall, though, so a
more accurate description might be "I may have said something that
can be (mis)interpreted like so". Sorry for the confusion.
In any case, an environment is not "completely hidden from users",
so it is not fundamentallly different from a command line option
anyway ;-)
> If that isn't what you intended that I can
> definitely add the option to git.c. And you would rather we perform the
> checking in git.c to see if a subcommand supports the prefix versus
> silently ignoring it if it hasn't? I'm assuming this checking would
> also be done in git.c?
I actually do not care strongly _where_ the check happens. It was
just that in the subcommand dispatcher would be the single place
that is easiest-to-implement to perform that check, that made me
suggest that. We already have various bits like NEEDS_WORK_TREE,
RUN_SETUP, etc. so REJECT_EXTERNAL_PREFIX (or whatever its name be;
I do not offhand recall current proposal) bit would fit there
naturally, I would think. Of course, non-built-in commands need to
protect themselves separately, if they want to.
^ permalink raw reply
* Re: [PATCH v2 04/11] i18n: add--interactive: mark plural strings
From: Junio C Hamano @ 2016-09-26 18:21 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1474913721.1035.9.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
>> > + } elsif ($did eq 'reversed') {
>> > + printf(Q__("reversed one path\n", "reversed %d paths\n",
>
> This should be 'reverted' not 'reversed'.
I'll mark v2 of this topic "not to be merged yet"; please send in a
corrected version 3 after you collect feedbacks from others and
adjusted the patches for them.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] recursive support for ls-files
From: Brandon Williams @ 2016-09-26 18:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqponqseya.fsf@gitster.mtv.corp.google.com>
On 09/26, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > In a previous email you mentioned that this feature should be completely
> > hidden from users, which is why I removed the command line option for
> > this latest series.
>
> I may have said something like that; I do not recall, though, so a
> more accurate description might be "I may have said something that
> can be (mis)interpreted like so". Sorry for the confusion.
>
> In any case, an environment is not "completely hidden from users",
> so it is not fundamentallly different from a command line option
> anyway ;-)
No worries, I'm still trying to get a hang of all this :)
True, that wouldn't be completely hidden either.
> > If that isn't what you intended that I can
> > definitely add the option to git.c. And you would rather we perform the
> > checking in git.c to see if a subcommand supports the prefix versus
> > silently ignoring it if it hasn't? I'm assuming this checking would
> > also be done in git.c?
>
> I actually do not care strongly _where_ the check happens. It was
> just that in the subcommand dispatcher would be the single place
> that is easiest-to-implement to perform that check, that made me
> suggest that. We already have various bits like NEEDS_WORK_TREE,
> RUN_SETUP, etc. so REJECT_EXTERNAL_PREFIX (or whatever its name be;
> I do not offhand recall current proposal) bit would fit there
> naturally, I would think. Of course, non-built-in commands need to
> protect themselves separately, if they want to.
That makes sense. I have an idea of where the check could be made. And
with those flags it may make sense to have the flag be an indicator that
the builtin is ready for submodule type commands ie SUPPORTS_SUBMODULES
or something along those lines. That way we only need to add the flag
to each command as we go (instead of all commands which don't support
it) and just make sure that the flag is set if the submodule prefix
option is being used. How does that sound?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 0/3] recursive support for ls-files
From: Junio C Hamano @ 2016-09-26 18:48 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git
In-Reply-To: <20160926183811.GA5123@google.com>
Brandon Williams <bmwill@google.com> writes:
> or something along those lines. That way we only need to add the flag
> to each command as we go ...
Sounds good. Thanks.
^ permalink raw reply
* Re: [PATCH v8 01/11] pkt-line: rename packet_write() to packet_write_fmt()
From: Lars Schneider @ 2016-09-26 18:49 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <592e18b7-ac2e-ed23-0321-57eb418d5172@gmail.com>
On 24 Sep 2016, at 23:14, Jakub Narębski <jnareb@gmail.com> wrote:
> Hello Lars,
>
> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> packet_write() should be called packet_write_fmt() as the string
>> parameter can be formatted.
>
> I would say:
>
> packet_write() should be called packet_write_fmt() because it
> is printf-like function where first parameter is format string.
>
> Or something like that. But such minor change might be not worth
> yet another reroll of this patch series.
>
> Perhaps it would be a good idea to explain the reasoning behind
> this change:
>
> This is important distinction to know from the name if the
> function accepts arbitrary binary data and/or arbitrary
> strings to be written - packet_write[_fmt()] do not.
packet_write() should be called packet_write_fmt() because it is a
printf-like function that takes a format string as first parameter.
packet_write_fmt() should be used for text strings only. Arbitrary
binary data should use a new packet_write() function that is introduced
in a subsequent patch.
Better?
>> pkt-line.h | 2 +-
>> shallow.c | 2 +-
>> upload-pack.c | 30 +++++++++++++++---------------
>> 11 files changed, 29 insertions(+), 29 deletions(-)
>
> Diffstat looks correct. Was the patch generated by doing search
> and replace?
Yes.
- Lars
^ permalink raw reply
* Re: [PATCH v8 02/11] pkt-line: extract set_packet_header()
From: Lars Schneider @ 2016-09-26 18:53 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <a5b9a46d-592e-089d-d156-36c4d61902c9@gmail.com>
On 24 Sep 2016, at 23:22, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Subject: [PATCH v8 02/11] pkt-line: extract set_packet_header()
>>
>> set_packet_header() converts an integer to a 4 byte hex string. Make
>> this function locally available so that other pkt-line functions can
>> use it.
>
> Ah. I have trouble understanding this commit message, as the
> set_packet_header() was not available before this patch, but it
> is good if one reads it together with commit summary / title.
>
> Writing
>
> Extracted set_packet_header() function converts...
>
> or
>
> New set_packet_header() function converts...
>
> would make it more clear, but it is all right as it is now.
> Perhaps also
>
> ... could use it.
>
> as currently no other pkt-line function but the one set_packet_header()
> was extracted from, namely format_packet(), uses it.
>
> But that is just nitpicking; no need to change on that account.
Changed it:
Extracted set_packet_header() function converts an integer to a 4 byte
hex string. Make this function locally available so that other pkt-line
functions could use it.
Thanks,
Lars
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox