Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind
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/XX\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/XX\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; } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind
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; > } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html