* [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind
@ 2012-07-28 15:46 Heiko Voigt
2012-07-30 0:54 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Heiko Voigt @ 2012-07-28 15:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Thomas Rast, Torsten Bögershausen
pfxlen can be longer than the path in objdir when relative_base contains
the path to gits object directory.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind
2012-07-28 15:46 [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind Heiko Voigt
@ 2012-07-30 0:54 ` Junio C Hamano
2012-07-30 1:01 ` Junio C Hamano
2012-07-30 17:33 ` [PATCH v2] " Heiko Voigt
0 siblings, 2 replies; 4+ messages in thread
From: Junio C Hamano @ 2012-07-30 0:54 UTC (permalink / raw)
To: Heiko Voigt; +Cc: git, Thomas Rast, Torsten Bögershausen
Heiko Voigt <hvoigt@hvoigt.net> 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 <hvoigt@hvoigt.net>
> ---
> 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;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind
2012-07-30 0:54 ` Junio C Hamano
@ 2012-07-30 1:01 ` Junio C Hamano
2012-07-30 17:33 ` [PATCH v2] " Heiko Voigt
1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2012-07-30 1:01 UTC (permalink / raw)
To: Heiko Voigt; +Cc: git, Thomas Rast, Torsten Bögershausen
Junio C Hamano <gitster@pobox.com> writes:
> Heiko Voigt <hvoigt@hvoigt.net> 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 <hvoigt@hvoigt.net>
>> ---
>> 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;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2] link_alt_odb_entry: fix read over array bounds reported by valgrind
2012-07-30 0:54 ` Junio C Hamano
2012-07-30 1:01 ` Junio C Hamano
@ 2012-07-30 17:33 ` Heiko Voigt
1 sibling, 0 replies; 4+ messages in thread
From: Heiko Voigt @ 2012-07-30 17:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Thomas Rast, Torsten Bögershausen
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 <hvoigt@hvoigt.net>
---
On Sun, Jul 29, 2012 at 05:54:02PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-07-30 17:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-28 15:46 [PATCH] link_alt_odb_entry: fix read over array bounds reported by valgrind Heiko Voigt
2012-07-30 0:54 ` Junio C Hamano
2012-07-30 1:01 ` Junio C Hamano
2012-07-30 17:33 ` [PATCH v2] " Heiko Voigt
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).