There are 9 places where prefix_path is called, and only in one of
them the returned pointer was checked to be non-zero and only to
call exit(128) as it is usually done by die(). In other 8 places,
the returned value was not checked and it caused SIGSEGV when a
path outside of the working tree was used. For instance, running
git update-index --add /some/path/outside
caused SIGSEGV.
This patch changes prefix_path() to die if the path is outside of
the repository, so it never returns NULL.
Signed-off-by: Dmitry Potapov <redacted>
---
setup.c | 9 ++-------
1 files changed, 2 insertions(+), 7 deletions(-)
Look at this change. Didn't the code error out before, too? Same in the
other cases. Hence, your patch subject does not describe the patch. And
I'd appreciate if you could at least show an example in the description
what the patch fixes.
-- Hannes
Look at this change. Didn't the code error out before, too?
It is certainly did not here. As to its caller, it depends. In fact,
there are two chunks like that in my patch, so I am not sure to which
one you refer here. If we speak about add_cacheinfo() then though the
function did not error out, its caller died with one of the following
messages:
git update-index: unable to update some-file-name
or
git update-index: --cacheinfo cannot add some-file-name
However, if we speak about add_index_entry_with_check then the caller
will not produce any error. The git would exit successfully (it still
does) and there was no error message as if everything was fine.
Perhaps, the exit code should be corrected too, but if the git just dies
when add_index_entry() fails it may cause that having one invalid path
will prevent to check out other files, which does not seem to be the
right thing to do.
As to correction to correction to make_cache_entry then after my
previous patch, it started to error out:
make_cache_entry failed for path 'some-file-name'
before that it silently segfaulted.
Same in the
other cases. Hence, your patch subject does not describe the patch.
Should I include the above explanation in the commit message or do you
have any objection to having the above error message in cases where the
caller already produce some message when it dies?
Dmitry
Look at this change. Didn't the code error out before, too?
It is certainly did not here. As to its caller, it depends. In fact,
there are two chunks like that in my patch, so I am not sure to which
one you refer here. If we speak about add_cacheinfo() then though the
function did not error out, its caller died with one of the following
messages:
git update-index: unable to update some-file-name
or
git update-index: --cacheinfo cannot add some-file-name
Look at the original patch. You did not change the behavior except to
write more error messages. Maybe I misunderstand the words "to error out".
I understand them as "to detect an error and return early", but not "write
an error message".
However, if we speak about add_index_entry_with_check then the caller
will not produce any error. The git would exit successfully (it still
does) and there was no error message as if everything was fine.
Perhaps, the exit code should be corrected too, but if the git just dies
when add_index_entry() fails it may cause that having one invalid path
will prevent to check out other files, which does not seem to be the
right thing to do.
As to correction to correction to make_cache_entry then after my
previous patch, it started to error out:
make_cache_entry failed for path 'some-file-name'
before that it silently segfaulted.
quoted
Same in the
other cases. Hence, your patch subject does not describe the patch.
Should I include the above explanation in the commit message or do you
have any objection to having the above error message in cases where the
caller already produce some message when it dies?
I don't object the change, only its (missing or IMHO incorrect)
justification. I don't think that the above text would be the correct
description because as far as I can see the only change you made was to
add error messages.
-- Hannes
If verification of path failed, it is always better to print an error message
saying this than relying on the caller function to print a meaningful error
message (especially when the callee already prints error message for another
situation).
Because the callers of add_index_entry_with_check() did not print any error
message, it resulted that the user would not notice the problem when checkout
if an invalid path failed.
Signed-off-by: Dmitry Potapov <redacted>
---
On Tue, Oct 07, 2008 at 08:03:46AM +0200, Johannes Sixt wrote:
Look at the original patch. You did not change the behavior except to
write more error messages. Maybe I misunderstand the words "to error out".
I understand them as "to detect an error and return early", but not "write
an error message".
For me, to "error out" means to show an error to the user. Usually, it
implies that the program will return after that, though not necessary
immediately. (Like gcc may print an error but it continues to parse the
program and may report more errors).
You are right that I have not changed anything in terms of exiting
earlier, and because I am aware about any commonly accepted definition
of what "error out" means, I have replaced the comment with less
ambiguous and detail description.
builtin-update-index.c | 2 +-
read-cache.c | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)