{"thread":{"id":"15784","subject":"[PATCH] make prefix_path() never return NULL","startedAt":"2008-10-05T00:40:36Z","lastAt":"2008-10-11T16:39:37Z","messageCount":7,"participants":["Dmitry Potapov","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"92314","messageId":"20081005004036.GO21650@dpotapov.dyndns.org","threadId":"15784","inReplyTo":null,"subject":"[PATCH] make prefix_path() never return NULL","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-05T00:40:36Z","receivedAt":"2008-10-05T00:40:36Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"There are 9 places where prefix_path is called, and only in one of\nthem the returned pointer was checked to be non-zero and only to\ncall exit(128) as it is usually done by die(). In other 8 places,\nthe returned value was not checked and it caused SIGSEGV when a\npath outside of the working tree was used. For instance, running\n  git update-index --add /some/path/outside\ncaused SIGSEGV.\n\nThis patch changes prefix_path() to die if the path is outside of\nthe repository, so it never returns NULL.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n setup.c |    9 ++-------\n 1 files changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 2e3248a..78a8041 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -110,9 +110,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)\n \t\tif (strncmp(sanitized, work_tree, len) ||\n \t\t    (sanitized[len] != '\\0' && sanitized[len] != '/')) {\n \t\terror_out:\n-\t\t\terror(\"'%s' is outside repository\", orig);\n-\t\t\tfree(sanitized);\n-\t\t\treturn NULL;\n+\t\t\tdie(\"'%s' is outside repository\", orig);\n \t\t}\n \t\tif (sanitized[len] == '/')\n \t\t\tlen++;\n@@ -216,10 +214,7 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \tprefixlen = prefix ? strlen(prefix) : 0;\n \twhile (*src) {\n \t\tconst char *p = prefix_path(prefix, prefixlen, *src);\n-\t\tif (p)\n-\t\t\t*(dst++) = p;\n-\t\telse\n-\t\t\texit(128); /* error message already given */\n+\t\t*(dst++) = p;\n \t\tsrc++;\n \t}\n \t*dst = NULL;\n-- \n1.6.0.2.445.g1198\n"},{"id":"92316","messageId":"1223172881-4948-1-git-send-email-dpotapov@gmail.com","threadId":"15784","inReplyTo":"20081005004036.GO21650@dpotapov.dyndns.org","subject":"[PATCH] do not segfault if make_cache_entry failed","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-05T02:14:40Z","receivedAt":"2008-10-05T02:14:40Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n builtin-apply.c    |    2 ++\n builtin-checkout.c |    2 ++\n builtin-reset.c    |    3 +++\n 3 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex e2c611b..342f2fe 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2586,6 +2586,8 @@ static void build_fake_ancestor(struct patch *list, const char *filename)\n \t\t\tsha1_ptr = sha1;\n \n \t\tce = make_cache_entry(patch->old_mode, sha1_ptr, name, 0, 0);\n+\t\tif (!ce)\n+\t\t\tdie(\"make_cache_entry failed for path '%s'\", name);\n \t\tif (add_index_entry(&result, ce, ADD_CACHE_OK_TO_ADD))\n \t\t\tdie (\"Could not add %s to temporary index\", name);\n \t}\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex b572b3b..3762f71 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -206,6 +206,8 @@ static int checkout_merged(int pos, struct checkout *state)\n \tce = make_cache_entry(create_ce_mode(active_cache[pos+1]->ce_mode),\n \t\t\t      sha1,\n \t\t\t      path, 2, 0);\n+\tif (!ce)\n+\t\tdie(\"make_cache_entry failed for path '%s'\", path);\n \tstatus = checkout_entry(ce, state, NULL);\n \treturn status;\n }\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex c24c219..16e6bb2 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -121,6 +121,9 @@ static void update_index_from_diff(struct diff_queue_struct *q,\n \t\t\tstruct cache_entry *ce;\n \t\t\tce = make_cache_entry(one->mode, one->sha1, one->path,\n \t\t\t\t0, 0);\n+\t\t\tif (!ce)\n+\t\t\t\tdie(\"make_cache_entry failed for path '%s'\",\n+\t\t\t\t    one->path);\n \t\t\tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD |\n \t\t\t\tADD_CACHE_OK_TO_REPLACE);\n \t\t} else\n-- \n1.6.0\n"},{"id":"92315","messageId":"1223172881-4948-2-git-send-email-dpotapov@gmail.com","threadId":"15784","inReplyTo":"1223172881-4948-1-git-send-email-dpotapov@gmail.com","subject":"[PATCH] error out if path is invalid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-05T02:14:41Z","receivedAt":"2008-10-05T02:14:41Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n builtin-update-index.c |    2 +-\n read-cache.c           |    6 ++++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 417f972..3a2291b 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -218,7 +218,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \tstruct cache_entry *ce;\n \n \tif (!verify_path(path))\n-\t\treturn -1;\n+\t\treturn error(\"Invalid path '%s'\", path);\n \n \tlen = strlen(path);\n \tsize = cache_entry_size(len);\ndiff --git a/read-cache.c b/read-cache.c\nindex 972592e..43dc338 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -591,8 +591,10 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \tint size, len;\n \tstruct cache_entry *ce;\n \n-\tif (!verify_path(path))\n+\tif (!verify_path(path)) {\n+\t\terror(\"Invalid path '%s'\", path);\n \t\treturn NULL;\n+\t}\n \n \tlen = strlen(path);\n \tsize = cache_entry_size(len);\n@@ -884,7 +886,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tif (!ok_to_add)\n \t\treturn -1;\n \tif (!verify_path(ce->name))\n-\t\treturn -1;\n+\t\treturn error(\"Invalid path '%s'\", ce->name);\n \n \tif (!skip_df_check &&\n \t    check_file_directory_conflict(istate, ce, pos, ok_to_replace)) {\n-- \n1.6.0\n"},{"id":"92391","messageId":"48E9B7FE.2000503@viscovery.net","threadId":"15784","inReplyTo":"1223172881-4948-2-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH] error out if path is invalid","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-06T07:02:22Z","receivedAt":"2008-10-06T07:02:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Dmitry Potapov schrieb:\n>  \tif (!verify_path(path))\n> -\t\treturn -1;\n> +\t\treturn error(\"Invalid path '%s'\", path);\n\nLook at this change. Didn't the code error out before, too? Same in the\nother cases. Hence, your patch subject does not describe the patch. And\nI'd appreciate if you could at least show an example in the description\nwhat the patch fixes.\n\n-- Hannes\n"},{"id":"92467","messageId":"20081007002221.GS21650@dpotapov.dyndns.org","threadId":"15784","inReplyTo":"48E9B7FE.2000503@viscovery.net","subject":"Re: [PATCH] error out if path is invalid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-07T00:22:21Z","receivedAt":"2008-10-07T00:22:21Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Oct 06, 2008 at 09:02:22AM +0200, Johannes Sixt wrote:\n> Dmitry Potapov schrieb:\n> >  \tif (!verify_path(path))\n> > -\t\treturn -1;\n> > +\t\treturn error(\"Invalid path '%s'\", path);\n> \n> Look at this change. Didn't the code error out before, too?\n\nIt is certainly did not here. As to its caller, it depends. In fact,\nthere are two chunks like that in my patch, so I am not sure to which\none you refer here. If we speak about add_cacheinfo() then though the\nfunction did not error out, its caller died with one of the following\nmessages:\n  git update-index: unable to update some-file-name\nor\n  git update-index: --cacheinfo cannot add some-file-name\n\nHowever, if we speak about add_index_entry_with_check then the caller\nwill not produce any error. The git would exit successfully (it still\ndoes) and there was no error message as if everything was fine.\n\nPerhaps, the exit code should be corrected too, but if the git just dies\nwhen add_index_entry() fails it may cause that having one invalid path\nwill prevent to check out other files, which does not seem to be the\nright thing to do.\n\nAs to correction to correction to make_cache_entry then after my\nprevious patch, it started to error out:\n\n  make_cache_entry failed for path 'some-file-name'\n\nbefore that it silently segfaulted.\n\n> Same in the\n> other cases. Hence, your patch subject does not describe the patch.\n\nShould I include the above explanation in the commit message or do you\nhave any objection to having the above error message in cases where the\ncaller already produce some message when it dies?\n\n\nDmitry\n"},{"id":"92489","messageId":"48EAFBC2.7020305@viscovery.net","threadId":"15784","inReplyTo":"20081007002221.GS21650@dpotapov.dyndns.org","subject":"Re: [PATCH] error out if path is invalid","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-07T06:03:46Z","receivedAt":"2008-10-07T06:03:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Dmitry Potapov schrieb:\n> On Mon, Oct 06, 2008 at 09:02:22AM +0200, Johannes Sixt wrote:\n>> Dmitry Potapov schrieb:\n>>>  \tif (!verify_path(path))\n>>> -\t\treturn -1;\n>>> +\t\treturn error(\"Invalid path '%s'\", path);\n>> Look at this change. Didn't the code error out before, too?\n> \n> It is certainly did not here. As to its caller, it depends. In fact,\n> there are two chunks like that in my patch, so I am not sure to which\n> one you refer here. If we speak about add_cacheinfo() then though the\n> function did not error out, its caller died with one of the following\n> messages:\n>   git update-index: unable to update some-file-name\n> or\n>   git update-index: --cacheinfo cannot add some-file-name\n\nLook at the original patch. You did not change the behavior except to\nwrite more error messages. Maybe I misunderstand the words \"to error out\".\nI understand them as \"to detect an error and return early\", but not \"write\nan error message\".\n\n> However, if we speak about add_index_entry_with_check then the caller\n> will not produce any error. The git would exit successfully (it still\n> does) and there was no error message as if everything was fine.\n> \n> Perhaps, the exit code should be corrected too, but if the git just dies\n> when add_index_entry() fails it may cause that having one invalid path\n> will prevent to check out other files, which does not seem to be the\n> right thing to do.\n> \n> As to correction to correction to make_cache_entry then after my\n> previous patch, it started to error out:\n> \n>   make_cache_entry failed for path 'some-file-name'\n> \n> before that it silently segfaulted.\n> \n>> Same in the\n>> other cases. Hence, your patch subject does not describe the patch.\n> \n> Should I include the above explanation in the commit message or do you\n> have any objection to having the above error message in cases where the\n> caller already produce some message when it dies?\n\nI don't object the change, only its (missing or IMHO incorrect)\njustification. I don't think that the above text would be the correct\ndescription because as far as I can see the only change you made was to\nadd error messages.\n\n-- Hannes\n"},{"id":"92801","messageId":"20081011163937.GA21650@dpotapov.dyndns.org","threadId":"15784","inReplyTo":"48EAFBC2.7020305@viscovery.net","subject":"[PATCH] print an error message for invalid path","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-10-11T16:39:37Z","receivedAt":"2008-10-11T16:39:37Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If verification of path failed, it is always better to print an error message\nsaying this than relying on the caller function to print a meaningful error\nmessage (especially when the callee already prints error message for another\nsituation).\n\nBecause the callers of add_index_entry_with_check() did not print any error\nmessage, it resulted that the user would not notice the problem when checkout\nif an invalid path failed.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n\nOn Tue, Oct 07, 2008 at 08:03:46AM +0200, Johannes Sixt wrote:\n> \n> Look at the original patch. You did not change the behavior except to\n> write more error messages. Maybe I misunderstand the words \"to error out\".\n> I understand them as \"to detect an error and return early\", but not \"write\n> an error message\".\n\nFor me, to \"error out\" means to show an error to the user. Usually, it\nimplies that the program will return after that, though not necessary\nimmediately. (Like gcc may print an error but it continues to parse the\nprogram and may report more errors).\n\nYou are right that I have not changed anything in terms of exiting\nearlier, and because I am aware about any commonly accepted definition\nof what \"error out\" means, I have replaced the comment with less\nambiguous and detail description.\n\n\n builtin-update-index.c |    2 +-\n read-cache.c           |    6 ++++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 417f972..3a2291b 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -218,7 +218,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \tstruct cache_entry *ce;\n \n \tif (!verify_path(path))\n-\t\treturn -1;\n+\t\treturn error(\"Invalid path '%s'\", path);\n \n \tlen = strlen(path);\n \tsize = cache_entry_size(len);\ndiff --git a/read-cache.c b/read-cache.c\nindex 901064b..aff6390 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -591,8 +591,10 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \tint size, len;\n \tstruct cache_entry *ce;\n \n-\tif (!verify_path(path))\n+\tif (!verify_path(path)) {\n+\t\terror(\"Invalid path '%s'\", path);\n \t\treturn NULL;\n+\t}\n \n \tlen = strlen(path);\n \tsize = cache_entry_size(len);\n@@ -874,7 +876,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tif (!ok_to_add)\n \t\treturn -1;\n \tif (!verify_path(ce->name))\n-\t\treturn -1;\n+\t\treturn error(\"Invalid path '%s'\", ce->name);\n \n \tif (!skip_df_check &&\n \t    check_file_directory_conflict(istate, ce, pos, ok_to_replace)) {\n-- \n1.6.0.2.447.g64b01\n"}]}