* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Jeff King @ 2011-10-20 4:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <7vvcrk9td7.fsf@alter.siamese.dyndns.org>
On Wed, Oct 19, 2011 at 09:31:16PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > And nothing shows up in the body, because git truncates at the NUL we
> > added:
> >
> > $ git show
> > commit 31337a1093af2d97eb2e6c08b261c2946395fdd3
> > Author: Jeff King <peff@peff.net>
> > Date: Wed Oct 19 15:34:00 2011 -0400
> >
> > 10
> >
> > diff --git a/file b/file
>
> But you cannot hide from "cat-file commit" ;-)
Yes. The implementation is a horrible hack, second only in grossness to
the original idea. :)
> With the recent push to more (perceived) security, it may probably make
> sense to teach "log" family commands to quote-show ^@ and what is behind
> in their output by default, perhaps with an option to turn it off.
Agreed. Having hidden cruft makes birthday collision attacks easier (or
it will, if sha1 ever gets broken to that point). Unfortunately, there
is a _ton_ of code which assumes that commit messages are
NUL-terminated, as they always have been since e871b64 (2005-05-25).
-Peff
^ permalink raw reply
* Re: How to verify that lines were only moved, not edited?
From: Johannes Sixt @ 2011-10-20 6:20 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111019163354.GB3157@sigill.intra.peff.net>
Am 10/19/2011 18:33, schrieb Jeff King:
> git init
> seq 1 5000 >foo
> git add foo
> git commit -m initial
> sed -i '/^2..$/d' foo
> seq 200 299 >>foo
> git commit -a -m 'move 200-299 to end'
>
> I get the expected result from "git blame -M" (i.e., everything
> attributed to the root commit).
I see. My example is more like this:
for i in `seq 1 20`; do md5sum - <<< $i; done > foo
git commit -a -m foo
for i in `seq 1 20`; do md5sum - <<< $i; done | sort > foo
git commit -a -m foo\ sorted
i.e., the sort order of a block of lines was changed "in place". Here,
most of the lines are attributed to the last commit. Am I expecting too
much from git-blame to detect line motions in such a case?
-- Hannes
^ permalink raw reply
* Re: How to verify that lines were only moved, not edited?
From: Johannes Sixt @ 2011-10-20 6:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvcrkdi59.fsf@alter.siamese.dyndns.org>
Am 10/19/2011 19:07, schrieb Junio C Hamano:
> When reviewing a "supposedly move-only" change, I typically just grab +/-
> blocks from the patch, remove the +/- prefix and run comparison between
> them.
Thanks. As I explained to Jeff, I don't have a block-move, but the sort
order of a block of lines was changed "in place". It seems I have to fall
back to a similarly manual method as well (e.g., compare the
whole-file-sorted versions of the pre- and the post-image).
-- Hannes
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Junio C Hamano @ 2011-10-20 6:57 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <20111020043448.GA7628@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Agreed. Having hidden cruft makes birthday collision attacks easier (or
> it will, if sha1 ever gets broken to that point). Unfortunately, there
> is a _ton_ of code which assumes that commit messages are
> NUL-terminated, as they always have been since e871b64 (2005-05-25).
I think that commit is irrelevant, as long as read_sha1_file() returns the
contents as <ptr,len> pair, which has been the case forever. It's just the
matter of propagating the length back up the callchain.
A naïve implementation to add "len" member to struct commit would increase
the size of the in-core commit object by sizeof(unsigned long), which we
may want to avoid. Traversals that care nothing but the topology of the
history would have to waste that memory and these things tend to add up
(8-byte ulong * 250k commits = 2MB).
Perhaps change the type of "buf" member in struct commit to a pointer to a
<ptr,len> pair, or something? Or perhaps a few megabytes wasted between
friends we do not care much about?
^ permalink raw reply
* Re: [msysGit] Re: Compiling on Windows
From: Philip Oakley @ 2011-10-20 7:06 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Andrew Ardill, Vincent van Ravesteijn, Git MsysGit, git
In-Reply-To: <alpine.DEB.1.00.1110191816500.32316@s15462909.onlinehome-server.info>
From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Sent: Thursday, October 20, 2011 12:17 AM
> On Wed, 19 Oct 2011, Philip Oakley wrote:
>
>> From: "Vincent van Ravesteijn" <vfr@lyx.org>
>> > Op 18-10-2011 6:08, Andrew Ardill schreef:
>> > > Hi list, I have been searching for details on what is required to
>> > > compile on Windows, but haven't found anything conclusive. Perhaps
>> > > there is something on the wiki, but unfortunately it is down at the
>> > > moment.
>
> The quickest way to get Git for Windows compiled is to download & run
> msysGit-netinstall from http://msysgit.googlecode.com/.
>
Johannes,
I've got the msysGit-netinstall, which is excellent, however it was the
additional setting up of MSVC (Microsoft Visual Studio) that I was trying to
get going as an IDE tool - Its a tool I have to use at work so I'm familiar
with it. I know that an MSVC compilation is not perfect..., but it would
allow me easier development before a proper gcc compilation.
The C:\msysgit\git\compat\vcbuild\README gave details of the extra parts to
download but it wasn't clear to me where I should put them within the C:\ or
C:\msysgit\ directory.
The msysGit-netinstall already includes git (C:\msysgit\git), so I wasn't
sure if I should download it a second time as instructed in the README. With
the improvements in msysGit-netinstall I expect that the README is slightly
out of date for the case where the netinstall was used. The blog post gave
an indication of _where_ the downloads were to be placed, but was otherwise
very similar to the README instructions.
regards
Philip
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Jeff King @ 2011-10-20 7:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <7vr5289mlu.fsf@alter.siamese.dyndns.org>
On Wed, Oct 19, 2011 at 11:57:17PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Agreed. Having hidden cruft makes birthday collision attacks easier (or
> > it will, if sha1 ever gets broken to that point). Unfortunately, there
> > is a _ton_ of code which assumes that commit messages are
> > NUL-terminated, as they always have been since e871b64 (2005-05-25).
>
> I think that commit is irrelevant, as long as read_sha1_file() returns the
> contents as <ptr,len> pair, which has been the case forever. It's just the
> matter of propagating the length back up the callchain.
It's not that the commit is bad or the source of problems. My point is
that the assumption that commit messages are NUL-terminated has been
there for a really long time, so there are lots of spots in the code
that sloppily run string functions on them. Every one of those needs to
be found and fixed (e.g., I remember seeing this in
for-each-ref.c:find_subpos recently).
It's not impossible, of course, or even really that hard. It's just a
giant pain, and I wonder if the effort is worth it.
> A naïve implementation to add "len" member to struct commit would increase
> the size of the in-core commit object by sizeof(unsigned long), which we
> may want to avoid. Traversals that care nothing but the topology of the
> history would have to waste that memory and these things tend to add up
> (8-byte ulong * 250k commits = 2MB).
>
> Perhaps change the type of "buf" member in struct commit to a pointer to a
> <ptr,len> pair, or something? Or perhaps a few megabytes wasted between
> friends we do not care much about?
I think you'd have to convert the struct (even if not every piece of
code is converted to use it) and profile.
-Peff
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Nguyen Thai Ngoc Duy @ 2011-10-20 7:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <7vr5289mlu.fsf@alter.siamese.dyndns.org>
On Thu, Oct 20, 2011 at 5:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Agreed. Having hidden cruft makes birthday collision attacks easier (or
>> it will, if sha1 ever gets broken to that point). Unfortunately, there
>> is a _ton_ of code which assumes that commit messages are
>> NUL-terminated, as they always have been since e871b64 (2005-05-25).
>
> I think that commit is irrelevant, as long as read_sha1_file() returns the
> contents as <ptr,len> pair, which has been the case forever. It's just the
> matter of propagating the length back up the callchain.
>
> A naïve implementation to add "len" member to struct commit would increase
> the size of the in-core commit object by sizeof(unsigned long), which we
> may want to avoid. Traversals that care nothing but the topology of the
> history would have to waste that memory and these things tend to add up
> (8-byte ulong * 250k commits = 2MB).
Or write commit_length(struct commit *c) that calculates SHA-1 from
c->buf and checks against c->object.sha1. If it does not match,
consider NUL part of the message and move to the next NUL. This
function would be expensive so we may not call frequently though.
Or store length in ((int*)c->buf)[-1].
--
Duy
^ permalink raw reply
* Re: [PATCH 08/12] do_for_each_ref_in_arrays(): new function
From: Michael Haggerty @ 2011-10-20 7:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <7vfwiobo75.fsf@alter.siamese.dyndns.org>
On 10/20/2011 12:39 AM, Junio C Hamano wrote:
> Is this necessary? IOW, is the helper function usable in any context
> other than merge-iterate loose and packed refs?
Soon the packed and cached refs will each be stored hierarchically, so
iteration through the combined caches will use a call to
do_for_each_ref_in_arrays() in each subdirectory, recursively.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 12/12] is_refname_available(): reimplement using do_for_each_ref_in_array()
From: Michael Haggerty @ 2011-10-20 7:46 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <7v4nz4bftk.fsf@alter.siamese.dyndns.org>
On 10/20/2011 03:40 AM, Junio C Hamano wrote:
> Hmm, why is this patch and only this one in the series full of whitespace
> violations? Did you use a different settings or something?
This happens rarely; I don't know why. Maybe I copy-pasted snippets
from a view in an application that expanded the tabs. Maybe emacs's
eliza program has achieved self-awareness and is punishing me for never
having properly learned elisp.
The incorrect lines are indented with 7, not 8, spaces so "tabify"
didn't help either.
I'll fix in reroll after I've received any other feedback.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser
From: Jonathan Nieder @ 2011-10-20 8:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List, Christian Couder
In-Reply-To: <7v8vogbgai.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> Check that the commit name argument to a "pick" or "revert" action in
>> '.git/sequencer/todo' is not too long
[...]
> Given that this function is going to be fixed properly so that it does not
> even need to use the "on-stack buffer", is this really necessary?
Right, I don't think it is. Keeping a testcase sounds worthwhile,
though.
^ permalink raw reply
* Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser
From: Ramkumar Ramachandra @ 2011-10-20 8:48 UTC (permalink / raw)
To: Jonathan Nieder, Junio C Hamano; +Cc: Git List, Christian Couder
In-Reply-To: <20111020080328.GA12337@elie.hsd1.il.comcast.net>
Hi Jonathan and Junio,
Jonathan Nieder writes:
> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>> Check that the commit name argument to a "pick" or "revert" action in
>>> '.git/sequencer/todo' is not too long
> [...]
>> Given that this function is going to be fixed properly so that it does not
>> even need to use the "on-stack buffer", is this really necessary?
>
> Right, I don't think it is. Keeping a testcase sounds worthwhile,
> though.
Okay. How about putting this after 5/6?
-- 8< --
Subject: [PATCH] t3510: guard against buffer overflows in parser
To guard against a buffer overflow in the parser, verify that
instruction sheets with overly long object names are parsed.
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t3510-cherry-pick-sequence.sh | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 0e29e03..39b55c1 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -12,6 +12,9 @@ test_description='Test cherry-pick continuation features
. ./test-lib.sh
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
pristine_detach () {
git cherry-pick --reset &&
git checkout -f "$1^0" &&
@@ -211,6 +214,17 @@ test_expect_success 'malformed instruction sheet 2' '
test_must_fail git cherry-pick --continue
'
+test_expect_success 'malformed instruction sheet 3' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "resolved" >foo &&
+ git add foo &&
+ git commit &&
+ sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ test_must_fail git cherry-pick --continue
+'
+
test_expect_success 'commit descriptions in insn sheet are optional' '
pristine_detach initial &&
test_must_fail git cherry-pick base..anotherpick &&
--
1.7.6.351.gb35ac.dirty
^ permalink raw reply related
* Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser
From: Jonathan Nieder @ 2011-10-20 9:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List, Christian Couder
In-Reply-To: <4E9FE061.3080601@gmail.com>
Ramkumar Ramachandra wrote:
> Okay. How about putting this after 5/6?
>
> -- 8< --
> Subject: [PATCH] t3510: guard against buffer overflows in parser
>
> To guard against a buffer overflow in the parser, verify that
> instruction sheets with overly long object names are parsed.
Looks good, except I would explain it differently, to avoid referring
to hypothetical implementation details ("What buffer overflow?"):
test: git cherry-pick --continue should cope with long object names
A naive implementation that uses a commit-id-shaped buffer
to store the word after "pick" in .git/sequencer/todo lines
would crash often. Our implementation is not so naive, but
add a test anyway to futureproof it.
Or:
test: make sure the "cherry-pick --continue" buffer overflow doesn't come back
Before commit ..., "git cherry-pick --continue" would overflow
under ... circumstance. Add a test to make sure it doesn't
happen again.
Though the implementation is actually better than that --- it can even
cope with a valid object name (e.g., a long name of a branch, or
something like "HEAD^{/refs.c: ensure struct whose member}") that is
that long, without truncating it. So if you have time for it, I think
it would be worth a test where the "git cherry-pick --continue"
succeeds, too.
Thanks,
Jonathan
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Nguyen Thai Ngoc Duy @ 2011-10-20 9:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <7vvcrk9td7.fsf@alter.siamese.dyndns.org>
On Thu, Oct 20, 2011 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> And nothing shows up in the body, because git truncates at the NUL we
>> added:
>>
>> $ git show
>> commit 31337a1093af2d97eb2e6c08b261c2946395fdd3
>> Author: Jeff King <peff@peff.net>
>> Date: Wed Oct 19 15:34:00 2011 -0400
>>
>> 10
>>
>> diff --git a/file b/file
>
> But you cannot hide from "cat-file commit" ;-)
>
> With the recent push to more (perceived) security, it may probably make
> sense to teach "log" family commands to quote-show ^@ and what is behind
> in their output by default, perhaps with an option to turn it off.
What about NUL in file name in tree objects? Suppose the original tree
has an entry named "goodthing". With luck, they might be able to
create a new tree object with the entry renamed to "evil\x001234" that
has the same SHA-1. Could that possibly cause any problems?
--
Duy
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Mikael Magnusson @ 2011-10-20 9:38 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð, Git Mailing List
In-Reply-To: <20111019190114.GA4670@sigill.intra.peff.net>
On 19 October 2011 21:01, Jeff King <peff@peff.net> wrote:
> On Wed, Oct 19, 2011 at 08:03:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> This is quick hack I wrote just before leaving work to show that I
>> could indeed push patches to our main repository starting with
>> 31337. Names hidden to protect the innocent.
>
> Clever and amusing.
>
>> Which in just over a minute will generate, in my case:
>>
>> $ git show --pretty=raw 313375d995e6f8b7773c6ed1ee165e5a9e15690b | head -n 7
>> commit 313375d995e6f8b7773c6ed1ee165e5a9e15690b
>> tree c9bebc99c05dfe61cccf02ebdf442945c8ff8b3c
>> parent 0dce2d45a79d26a593f0e12301cdfeb7eb23c17a
>> author Ævar Arnfjörð Bjarmason <avar@example.com> <censored> <censored>
>> committer Ævar Arnfjörð Bjarmason <avar@example.com> <censored> <censored>
>> lulz 697889
>
> Nice header name.
If you don't mind waiting, you could just increase the timestamps
until you get the desired collision. (If you still want them to be
correct, trying 4000000 times would about 6 weeks though :).
--
Mikael Magnusson
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Ted Ts'o @ 2011-10-20 13:14 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Git Mailing List
In-Reply-To: <20111020071356.GA14945@sigill.intra.peff.net>
On Thu, Oct 20, 2011 at 03:13:56AM -0400, Jeff King wrote:
> It's not that the commit is bad or the source of problems. My point is
> that the assumption that commit messages are NUL-terminated has been
> there for a really long time, so there are lots of spots in the code
> that sloppily run string functions on them. Every one of those needs to
> be found and fixed (e.g., I remember seeing this in
> for-each-ref.c:find_subpos recently).
Another possibility is to warn if the commit messages are not NULL
terminated. Note though that if we're really worried about a bad guy
trying to attack us with a hash collision, he/she could always use
"invisible" non-printing characters in the commit message, and/or just
mess with one or both of the timestamps. The more bits and more
degrees of flexibility the attacker has, the easier it would be, of
course. In the grand scheme of things it's not clear to me how big of
a deal this would be.
If people were really concerned it would probably be easier to use
backup crypto checksum using something stronger (say, SHA-2 or the
eventual SHA-3). Just store the backup checksums of the parent
commitments in some backwardly compatible place that old git
implementations wouldn't look (say, after the NULL in the commit
message if there isn't a better place :-), and new implementations
would know to generate the checksums, and old implementations would
ignore it.
That way, if anyone *does* figure out a way to generate real hash
collisions with SHA1 of objects that look almost completely identical
to the original objects, new implementations that would gradually make
their way out to the field could verify the SHA-2 (or SHA-3, when it
is announced, assuming that we the tag the checksums with a type
identifier) checksums and notice that they are not correct.
Maybe someone's already thought of this, but the cool thing about this
idea is it's a way that we can upgrade to a stronger hash algorithm
without needing a flag day due to some kind of incompatible format
change; we keep using SHA1 for the user-visible hash, since it's fine
modulo intentional attacks, and then use a hidden, backup checksum
which can be checked either all the time, or if that turns out to be a
computational burden, at some configurable random percent of the time.
Anyway, just a thought....
- Ted
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Elijah Newren @ 2011-10-20 13:44 UTC (permalink / raw)
To: Mikael Magnusson
Cc: Jeff King, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <CAHYJk3QV8QckbOM76QfqQfTsyOtk+nvfhCped+W3t60JfCfouA@mail.gmail.com>
On Thu, Oct 20, 2011 at 3:38 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
>> On Wed, Oct 19, 2011 at 08:03:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>> This is quick hack I wrote just before leaving work to show that I
>>> could indeed push patches to our main repository starting with
>>> 31337. Names hidden to protect the innocent.
>>
> If you don't mind waiting, you could just increase the timestamps
> until you get the desired collision. (If you still want them to be
> correct, trying 4000000 times would about 6 weeks though :).
But the nice thing is that we have both author and committer dates to
twiddle with, meaning that if we need 4000000 different values to try
then it's only 2000 for each of those two dates, i.e. we only need to
be willing to let those dates float by about half an hour.
^ permalink raw reply
* Tracking cherry picks
From: Phillip Susi @ 2011-10-20 14:21 UTC (permalink / raw)
To: git
I need to maintain a few stable release branches in addition to the
master branch. Sometimes a bug is found and the fix needs applied to
multiple branches. I would like to be able to list what branches the
fix has been applied to to validate that it went in everywhere it was
needed, but after cherry-picking the fix from master to the stable
branches, the SHA1 of the commit is different, and so git branch
--contains does not think the commit was applied to each of the stable
branches.
Is there a way around this? Why doesn't git-cherrypick record the
original SHA1 it was picked from in the commit?
^ permalink raw reply
* Re: Tracking cherry picks
From: Kirill Likhodedov @ 2011-10-20 15:00 UTC (permalink / raw)
To: Phillip Susi; +Cc: git
In-Reply-To: <4EA02E6C.2040608@cfl.rr.com>
20.10.2011, в 18:21, Phillip Susi:
> Why doesn't git-cherrypick record the original SHA1 it was picked from in the commit?
It does if you specify "-x" option to cherry-pick
See the man for git-cherry-pick:
-x
When recording the commit, append a line that says "(cherry picked from commit ...)" to the
original commit message in order to indicate which commit this change was cherry-picked from.
This is done only for cherry picks without conflicts. Do not use this option if you are
cherry-picking from your private branch because the information is useless to the recipient. If
on the other hand you are cherry-picking between two publicly visible branches (e.g. backporting
a fix to a maintenance branch for an older release from a development branch), adding this
information can be useful.
^ permalink raw reply
* Re: Tracking cherry picks
From: Ramkumar Ramachandra @ 2011-10-20 15:34 UTC (permalink / raw)
To: Kirill Likhodedov; +Cc: Phillip Susi, git
In-Reply-To: <37162B20-4758-433E-B11E-CE4B7FF27FBA@gmail.com>
Hi,
Kirill Likhodedov writes:
> 20.10.2011, в 18:21, Phillip Susi:
>> Why doesn't git-cherrypick record the original SHA1 it was picked from in the commit?
>
> It does if you specify "-x" option to cherry-pick
> See the man for git-cherry-pick:
> [...]
Right. As an interesting historical note, git has been omitting the
original object name by default when cherry-picking/ reverting since
abd6970a (cherry-pick: make -r the default, 2006-10-05).
-- Ram
^ permalink raw reply
* Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser
From: Ramkumar Ramachandra @ 2011-10-20 15:36 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List, Christian Couder
In-Reply-To: <20111020090912.GA21471@elie.hsd1.il.comcast.net>
Hi Jonathan,
Jonathan Nieder writes:
> [...]
> Looks good, except I would explain it differently, to avoid referring
> to hypothetical implementation details ("What buffer overflow?"):
>
> test: git cherry-pick --continue should cope with long object names
>
> A naive implementation that uses a commit-id-shaped buffer
> to store the word after "pick" in .git/sequencer/todo lines
> would crash often. Our implementation is not so naive, but
> add a test anyway to futureproof it.
> [...]
I picked this one.
> Though the implementation is actually better than that --- it can even
> cope with a valid object name (e.g., a long name of a branch, or
> something like "HEAD^{/refs.c: ensure struct whose member}") that is
> that long, without truncating it. So if you have time for it, I think
> it would be worth a test where the "git cherry-pick --continue"
> succeeds, too.
Good idea. Will re-roll shortly.
Thanks.
-- Ram
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Jeff King @ 2011-10-20 15:44 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <CACsJy8B7CJ3VO-UKCym2kgfOOPadL25gt2sxApk95nKoWVk2yQ@mail.gmail.com>
On Thu, Oct 20, 2011 at 08:14:56PM +1100, Nguyen Thai Ngoc Duy wrote:
> > But you cannot hide from "cat-file commit" ;-)
> >
> > With the recent push to more (perceived) security, it may probably make
> > sense to teach "log" family commands to quote-show ^@ and what is behind
> > in their output by default, perhaps with an option to turn it off.
>
> What about NUL in file name in tree objects? Suppose the original tree
> has an entry named "goodthing". With luck, they might be able to
> create a new tree object with the entry renamed to "evil\x001234" that
> has the same SHA-1. Could that possibly cause any problems?
NUL is already meaningful in a tree object; it is the end of the
filename. So after the NUL, we will consider the next 20 bytes to be
sha1, and then after that, the mode of the next file entry.
-Peff
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Jeff King @ 2011-10-20 15:56 UTC (permalink / raw)
To: Ted Ts'o
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Git Mailing List
In-Reply-To: <20111020131454.GB7464@thunk.org>
On Thu, Oct 20, 2011 at 09:14:55AM -0400, Ted Ts'o wrote:
> Another possibility is to warn if the commit messages are not NULL
> terminated.
A minor nit, but it's not whether they are terminated with NUL, but
rather whether they have embedded NUL. But yeah, this could maybe just
be something fsck looks for.
> Note though that if we're really worried about a bad guy trying to
> attack us with a hash collision, he/she could always use "invisible"
> non-printing characters in the commit message, and/or just mess with
> one or both of the timestamps. The more bits and more degrees of
> flexibility the attacker has, the easier it would be, of course. In
> the grand scheme of things it's not clear to me how big of a deal this
> would be.
Good point. Append-only attacks are cheaper, because you can avoid doing
most of the hash computation on each iteration (like my patch does). But
that's not a big-O speedup, it just makes the constant smaller. So you
could assume that any feasible appending attack would probably become
feasible for recomputing the full hash eventually.
> If people were really concerned it would probably be easier to use
> backup crypto checksum using something stronger (say, SHA-2 or the
> eventual SHA-3). Just store the backup checksums of the parent
> commitments in some backwardly compatible place that old git
> implementations wouldn't look (say, after the NULL in the commit
> message if there isn't a better place :-), and new implementations
> would know to generate the checksums, and old implementations would
> ignore it.
Yeah, if birthday attacks against sha1 become possible, the sensible
thing is probably not to worry too much about the file format, but to
use a better hash.
Commits can hide extra hashes in a header pretty easily. But what about
trees and blobs? I don't think there's any "ignored" space in either
one.
-Peff
^ permalink raw reply
* [PATCH] git-remote-mediawiki: don't include HTTP login/password in author
From: Matthieu Moy @ 2011-10-20 17:04 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
On the MediaWiki side, the author information is just the MediaWiki login
of the contributor. The import turns it into login@$wiki_name to create
the author's email address on the wiki side. But we don't want this to
include the HTTP password if it's present in the URL ...
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
contrib/mw-to-git/git-remote-mediawiki | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index 0b32d18..c18bfa1 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -109,6 +109,10 @@ $dumb_push = ($dumb_push eq "true");
my $wiki_name = $url;
$wiki_name =~ s/[^\/]*:\/\///;
+# If URL is like http://user:password@example.com/, we clearly don't
+# want the password in $wiki_name. While we're there, also remove user
+# and '@' sign, to avoid author like MWUser@HTTPUser@host.com
+$wiki_name =~ s/^.*@//;
# Commands parser
my $entry;
--
1.7.7.140.ge3099
^ permalink raw reply related
* [PATCH] builtin/pack-objects.c: Fix a printf format compiler warning
From: Ramsay Jones @ 2011-10-19 19:10 UTC (permalink / raw)
To: dpmcgee; +Cc: Junio C Hamano, GIT Mailing-list
In particular, on systems that define uint32_t as an unsigned long,
gcc complains as follows:
CC builtin/pack-objects.o
pack-objects.c: In function `compute_write_order':
pack-objects.c:600: warning: unsigned int format, uint32_t arg (arg 3)
In order to suppress the warning, we use the C99 format specifier
macro PRIu32.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Dan,
If you need to re-roll your pack-objects series (dm/pack-objects-update
branch in pu), could you please squash this change into your final commit
0a8145bd (pack-objects: don't traverse objects unnecessarily, 18-10-2011).
If you don't need to re-roll, then I'm hoping Junio will notice and squash
this in before it hits next. ;-)
ATB,
Ramsay Jones
builtin/pack-objects.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6db45fa..4bbd815 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -597,7 +597,7 @@ static struct object_entry **compute_write_order(void)
}
if (wo_end != nr_objects)
- die("ordered %u objects, expected %u", wo_end, nr_objects);
+ die("ordered %u objects, expected %"PRIu32, wo_end, nr_objects);
return wo;
}
--
1.7.7
^ permalink raw reply related
* Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser
From: Junio C Hamano @ 2011-10-20 17:15 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ramkumar Ramachandra, Git List, Christian Couder
In-Reply-To: <20111020090912.GA21471@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Looks good, except I would explain it differently, to avoid referring
> to hypothetical implementation details ("What buffer overflow?"):
>
> test: git cherry-pick --continue should cope with long object names
>
> A naive implementation that uses a commit-id-shaped buffer
> to store the word after "pick" in .git/sequencer/todo lines
> would crash often. Our implementation is not so naive, but
> add a test anyway to futureproof it.
>
> Or:
>
> test: make sure the "cherry-pick --continue" buffer overflow doesn't come back
>
> Before commit ..., "git cherry-pick --continue" would overflow
> under ... circumstance. Add a test to make sure it doesn't
> happen again.
I doubt you would need any of that.
You can just explain the commit that stops copying the lines into a
private, fixed buffer a bit better (e.g. "such copying is not just
wasteful but is wrong by unnecessary placing an artificial limit on the
line length"), and say "Incidentally, this fixes a bug in the earlier
round of this series that failed to read lines that are too long to fit on
the buffer, demonstrated by the test added by this patch", or something.
Then the additional test can become part of the patch that corrects the
parsing logic, no?
^ 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