{"thread":{"id":"31124","subject":"[PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind","startedAt":"2012-07-28T15:46:36Z","lastAt":"2012-07-30T17:33:29Z","messageCount":4,"participants":["Heiko Voigt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"196032","messageId":"20120728154635.GB98893@book.hvoigt.net","threadId":"31124","inReplyTo":null,"subject":"[PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-07-28T15:46:36Z","receivedAt":"2012-07-28T15:46:36Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"pfxlen can be longer than the path in objdir when relative_base contains\nthe path to gits object directory.\n\nSigned-off-by: Heiko Voigt \n---\n sha1_file.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4ccaf7a..631d0dd 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -251,7 +251,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \tconst char *objdir = get_object_directory();\n \tstruct alternate_object_database *ent;\n \tstruct alternate_object_database *alt;\n-\tint pfxlen, entlen;\n+\tint pfxlen, entlen, objdirlen;\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n \tif (!is_absolute_path(entry) && relative_base) {\n@@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (!memcmp(ent->base, objdir, pfxlen)) {\n+\tobjdirlen = strlen(objdir);\n+\tif (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) {\n \t\tfree(ent);\n \t\treturn -1;\n \t}\n-- \n1.7.12.rc0.23.gc9a5ac4\n"},{"id":"196092","messageId":"7v4noqul05.fsf@alter.siamese.dyndns.org","threadId":"31124","inReplyTo":"20120728154635.GB98893@book.hvoigt.net","subject":"Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-30T00:54:02Z","receivedAt":"2012-07-30T00:54:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> pfxlen can be longer than the path in objdir when relative_base contains\n> the path to gits object directory.\n\ns/gits/????/ perhaps \"Git's\", but I am not sure.\n\n> Signed-off-by: Heiko Voigt \n> ---\n> sha1_file.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 4ccaf7a..631d0dd 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -251,7 +251,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n> \tconst char *objdir = get_object_directory();\n> \tstruct alternate_object_database *ent;\n> \tstruct alternate_object_database *alt;\n> -\tint pfxlen, entlen;\n> +\tint pfxlen, entlen, objdirlen;\n> \tstruct strbuf pathbuf = STRBUF_INIT;\n> \n> \tif (!is_absolute_path(entry) && relative_base) {\n> @@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n> \t\t\treturn -1;\n> \t\t}\n> \t}\n> -\tif (!memcmp(ent->base, objdir, pfxlen)) {\n> +\tobjdirlen = strlen(objdir);\n> +\tif (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) {\n\nThe new code tells us to compare up to the shorter length between\nobjdir (i.e. path/to/.git/objects) and the given alternate object\ndirectory (i.e. alt/path/to/.git/objects), but is that really what\nwe want? What happens if the given alternate object directory were\n\"path/to/.git/objects-not-quite\", with objdir \"path/to/.git/objects\"?\n\nThey are not the same directory, and this check is about avoiding\n\"the common mistake of listing ... object directory itself\", no?\n\n> \t\tfree(ent);\n> \t\treturn -1;\n> \t}\n"},{"id":"196094","messageId":"7vzk6it62h.fsf@alter.siamese.dyndns.org","threadId":"31124","inReplyTo":"7v4noqul05.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-30T01:01:58Z","receivedAt":"2012-07-30T01:01:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Heiko Voigt writes:\n>\n>> pfxlen can be longer than the path in objdir when relative_base contains\n>> the path to gits object directory.\n>\n> s/gits/????/ perhaps \"Git's\", but I am not sure.\n>\n>> Signed-off-by: Heiko Voigt \n>> ---\n>> sha1_file.c | 5 +++--\n>> 1 file changed, 3 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 4ccaf7a..631d0dd 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> \t\t\treturn -1;\n>> \t\t}\n>> \t}\n>> -\tif (!memcmp(ent->base, objdir, pfxlen)) {\n>> +\tobjdirlen = strlen(objdir);\n>> +\tif (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) {\n>\n> The new code tells us to compare up to the shorter length between\n> objdir (i.e. path/to/.git/objects) and the given alternate object\n> directory (i.e. alt/path/to/.git/objects), but is that really what\n> we want? What happens if the given alternate object directory were\n> \"path/to/.git/objects-not-quite\", with objdir \"path/to/.git/objects\"?\n>\n> They are not the same directory, and this check is about avoiding\n> \"the common mistake of listing ... object directory itself\", no?\n>\n>> \t\tfree(ent);\n>> \t\treturn -1;\n>> \t}\n\nIn other words, wouldn't this be sufficient? We NUL terminate\nent->base[pfxlen] when we prepare that buffer with\n\n\tLEADING PATH\\0XX/XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\\0\n\nin preparation for these \"duplicate check\" step, and then we turn\nthe NUL at ent->base[pfxlen] to '/' before leaving the function to\nmake it\n\n\tLEADING PATH/XX/XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\\0\n\nso that we can fill XX when probing for loose objects.\n\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4f06a0e..a1f3bee 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -298,7 +298,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (!memcmp(ent->base, objdir, pfxlen)) {\n+\tif (!strcmp(ent->base, objdir)) {\n \t\tfree(ent);\n \t\treturn -1;\n \t}\n"},{"id":"196169","messageId":"20120730173328.GA28420@book.hvoigt.net","threadId":"31124","inReplyTo":"7v4noqul05.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] link_alt_odb_entry: fix read over array bounds reported by valgrind","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-07-30T17:33:29Z","receivedAt":"2012-07-30T17:33:29Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"pfxlen can be longer than the path in objdir when relative_base contains\nthe path to Git's object directory.\n\nSigned-off-by: Heiko Voigt \n---\n\nOn Sun, Jul 29, 2012 at 05:54:02PM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > pfxlen can be longer than the path in objdir when relative_base contains\n> > the path to gits object directory.\n> \n> s/gits/????/ perhaps \"Git's\", but I am not sure.\n\nGit's is correct.\n\n> > @@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n> > \t\t\treturn -1;\n> > \t\t}\n> > \t}\n> > -\tif (!memcmp(ent->base, objdir, pfxlen)) {\n> > +\tobjdirlen = strlen(objdir);\n> > +\tif (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) {\n> \n> The new code tells us to compare up to the shorter length between\n> objdir (i.e. path/to/.git/objects) and the given alternate object\n> directory (i.e. alt/path/to/.git/objects), but is that really what\n> we want? What happens if the given alternate object directory were\n> \"path/to/.git/objects-not-quite\", with objdir \"path/to/.git/objects\"?\n> \n> They are not the same directory, and this check is about avoiding\n> \"the common mistake of listing ... object directory itself\", no?\n\nYou are right. strcmp is the correct solution for this. I tried your\nchange and it also fixes the error valgrind reported.\n\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4ccaf7a..af5cfbd 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -298,7 +298,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (!memcmp(ent->base, objdir, pfxlen)) {\n+\tif (!strcmp(ent->base, objdir)) {\n \t\tfree(ent);\n \t\treturn -1;\n \t}\n-- \n1.7.12.rc0.24.g3fa2a49\n"}]}