# [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind 4 messages from 2012-07-28 to 2012-07-30. Participants: Heiko Voigt, Junio C Hamano. Thread: https://gitlist.dev/t/31124 ## Heiko Voigt, 2012-07-28 15:46 Subject: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind Message-ID: <20120728154635.GB98893@book.hvoigt.net> URL: https://gitlist.dev/e/20120728154635.GB98893%40book.hvoigt.net ``` pfxlen can be longer than the path in objdir when relative_base contains the path to gits object directory. Signed-off-by: Heiko Voigt --- sha1_file.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 4ccaf7a..631d0dd 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -251,7 +251,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative const char *objdir = get_object_directory(); struct alternate_object_database *ent; struct alternate_object_database *alt; - int pfxlen, entlen; + int pfxlen, entlen, objdirlen; struct strbuf pathbuf = STRBUF_INIT; if (!is_absolute_path(entry) && relative_base) { @@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative return -1; } } - if (!memcmp(ent->base, objdir, pfxlen)) { + objdirlen = strlen(objdir); + if (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) { free(ent); return -1; } -- 1.7.12.rc0.23.gc9a5ac4 ``` ## Junio C Hamano, 2012-07-30 00:54 Subject: Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind Message-ID: <7v4noqul05.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4noqul05.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120728154635.GB98893@book.hvoigt.net> ``` Heiko Voigt writes: > pfxlen can be longer than the path in objdir when relative_base contains > the path to gits object directory. s/gits/????/ perhaps "Git's", but I am not sure. > Signed-off-by: Heiko Voigt > --- > sha1_file.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/sha1_file.c b/sha1_file.c > index 4ccaf7a..631d0dd 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -251,7 +251,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative > const char *objdir = get_object_directory(); > struct alternate_object_database *ent; > struct alternate_object_database *alt; > - int pfxlen, entlen; > + int pfxlen, entlen, objdirlen; > struct strbuf pathbuf = STRBUF_INIT; > > if (!is_absolute_path(entry) && relative_base) { > @@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative > return -1; > } > } > - if (!memcmp(ent->base, objdir, pfxlen)) { > + objdirlen = strlen(objdir); > + if (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) { The new code tells us to compare up to the shorter length between objdir (i.e. path/to/.git/objects) and the given alternate object directory (i.e. alt/path/to/.git/objects), but is that really what we want? What happens if the given alternate object directory were "path/to/.git/objects-not-quite", with objdir "path/to/.git/objects"? They are not the same directory, and this check is about avoiding "the common mistake of listing ... object directory itself", no? > free(ent); > return -1; > } ``` ## Junio C Hamano, 2012-07-30 01:01 Subject: Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind Message-ID: <7vzk6it62h.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzk6it62h.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v4noqul05.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Heiko Voigt writes: > >> pfxlen can be longer than the path in objdir when relative_base contains >> the path to gits object directory. > > s/gits/????/ perhaps "Git's", but I am not sure. > >> Signed-off-by: Heiko Voigt >> --- >> sha1_file.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/sha1_file.c b/sha1_file.c >> index 4ccaf7a..631d0dd 100644 >> --- a/sha1_file.c >> +++ b/sha1_file.c >> return -1; >> } >> } >> - if (!memcmp(ent->base, objdir, pfxlen)) { >> + objdirlen = strlen(objdir); >> + if (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) { > > The new code tells us to compare up to the shorter length between > objdir (i.e. path/to/.git/objects) and the given alternate object > directory (i.e. alt/path/to/.git/objects), but is that really what > we want? What happens if the given alternate object directory were > "path/to/.git/objects-not-quite", with objdir "path/to/.git/objects"? > > They are not the same directory, and this check is about avoiding > "the common mistake of listing ... object directory itself", no? > >> free(ent); >> return -1; >> } In other words, wouldn't this be sufficient? We NUL terminate ent->base[pfxlen] when we prepare that buffer with LEADING PATH\0XX/XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\0 in preparation for these "duplicate check" step, and then we turn the NUL at ent->base[pfxlen] to '/' before leaving the function to make it LEADING PATH/XX/XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\0 so that we can fill XX when probing for loose objects. sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index 4f06a0e..a1f3bee 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -298,7 +298,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative return -1; } } - if (!memcmp(ent->base, objdir, pfxlen)) { + if (!strcmp(ent->base, objdir)) { free(ent); return -1; } ``` ## Heiko Voigt, 2012-07-30 17:33 Subject: [PATCH v2] link_alt_odb_entry: fix read over array bounds reported by valgrind Message-ID: <20120730173328.GA28420@book.hvoigt.net> URL: https://gitlist.dev/e/20120730173328.GA28420%40book.hvoigt.net In-Reply-To: <7v4noqul05.fsf@alter.siamese.dyndns.org> ``` pfxlen can be longer than the path in objdir when relative_base contains the path to Git's object directory. Signed-off-by: Heiko Voigt --- On Sun, Jul 29, 2012 at 05:54:02PM -0700, Junio C Hamano wrote: > Heiko Voigt writes: > > > pfxlen can be longer than the path in objdir when relative_base contains > > the path to gits object directory. > > s/gits/????/ perhaps "Git's", but I am not sure. Git's is correct. > > @@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative > > return -1; > > } > > } > > - if (!memcmp(ent->base, objdir, pfxlen)) { > > + objdirlen = strlen(objdir); > > + if (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) { > > The new code tells us to compare up to the shorter length between > objdir (i.e. path/to/.git/objects) and the given alternate object > directory (i.e. alt/path/to/.git/objects), but is that really what > we want? What happens if the given alternate object directory were > "path/to/.git/objects-not-quite", with objdir "path/to/.git/objects"? > > They are not the same directory, and this check is about avoiding > "the common mistake of listing ... object directory itself", no? You are right. strcmp is the correct solution for this. I tried your change and it also fixes the error valgrind reported. sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index 4ccaf7a..af5cfbd 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -298,7 +298,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative return -1; } } - if (!memcmp(ent->base, objdir, pfxlen)) { + if (!strcmp(ent->base, objdir)) { free(ent); return -1; } -- 1.7.12.rc0.24.g3fa2a49 ```