# [PATCH] Off-by-one error in get_path_prefix(), found by Valgrind 4 messages from 2006-06-07 to 2006-06-07. Participants: Pavel Roskin, Rene Scharfe, Junio C Hamano. Thread: https://gitlist.dev/t/4420 ## Pavel Roskin, 2006-06-07 17:01 Subject: [PATCH] Off-by-one error in get_path_prefix(), found by Valgrind Message-ID: <20060607170140.13372.64613.stgit@dv.roinet.com> URL: https://gitlist.dev/e/20060607170140.13372.64613.stgit%40dv.roinet.com ``` From: Pavel Roskin Signed-off-by: Pavel Roskin --- builtin-tar-tree.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c index 5f740cf..05da1f2 100644 --- a/builtin-tar-tree.c +++ b/builtin-tar-tree.c @@ -166,8 +166,8 @@ static unsigned int ustar_header_chksum( static int get_path_prefix(const struct strbuf *path, int maxlen) { int i = path->len; - if (i > maxlen) - i = maxlen; + if (i >= maxlen) + i = maxlen - 1; while (i > 0 && path->buf[i] != '/') i--; return i; ``` ## Rene Scharfe, 2006-06-07 18:05 Subject: Re: [PATCH] Off-by-one error in get_path_prefix(), found by Valgrind Message-ID: <20060607180543.GA26638@lsrfire.ath.cx> URL: https://gitlist.dev/e/20060607180543.GA26638%40lsrfire.ath.cx In-Reply-To: <20060607170140.13372.64613.stgit@dv.roinet.com> ``` On Wed, Jun 07, 2006 at 01:01:40PM -0400, Pavel Roskin wrote: > From: Pavel Roskin > > Signed-off-by: Pavel Roskin > --- > > builtin-tar-tree.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c > index 5f740cf..05da1f2 100644 > --- a/builtin-tar-tree.c > +++ b/builtin-tar-tree.c > @@ -166,8 +166,8 @@ static unsigned int ustar_header_chksum( > static int get_path_prefix(const struct strbuf *path, int maxlen) > { > int i = path->len; > - if (i > maxlen) > - i = maxlen; > + if (i >= maxlen) > + i = maxlen - 1; > while (i > 0 && path->buf[i] != '/') > i--; > return i; Argh, yes. Thanks, Pavel! However, the other branch is incorrect, too: accessing path->buf[path->len] is wrong, even if it's within the buffer. In order to use a length variable to point to the end of some string we need to subtract 1. *sigh* So, how about this one instead? Signed-off-by: Rene Scharfe diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c index 5f740cf..7663b9b 100644 --- a/builtin-tar-tree.c +++ b/builtin-tar-tree.c @@ -168,8 +168,9 @@ static int get_path_prefix(const struct int i = path->len; if (i > maxlen) i = maxlen; - while (i > 0 && path->buf[i] != '/') + do { i--; + } while (i > 0 && path->buf[i] != '/'); return i; } ``` ## Pavel Roskin, 2006-06-07 18:33 Subject: Re: [PATCH] Off-by-one error in get_path_prefix(), found by Valgrind Message-ID: <1149705204.11931.3.camel@dv> URL: https://gitlist.dev/e/1149705204.11931.3.camel%40dv In-Reply-To: <20060607180543.GA26638@lsrfire.ath.cx> ``` On Wed, 2006-06-07 at 20:05 +0200, Rene Scharfe wrote: > Argh, yes. Thanks, Pavel! Actually, thanks to Julian Seward for Valgrind. > However, the other branch is incorrect, too: > accessing path->buf[path->len] is wrong, even if it's within the buffer. > In order to use a length variable to point to the end of some string we > need to subtract 1. *sigh* So, how about this one instead? Fine with me. Thank you for noticing! -- Regards, Pavel Roskin ``` ## Junio C Hamano, 2006-06-07 18:47 Subject: Re: [PATCH] Off-by-one error in get_path_prefix(), found by Valgrind Message-ID: <7v8xo8hkgo.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v8xo8hkgo.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <1149705204.11931.3.camel@dv> ``` Pavel Roskin writes: > On Wed, 2006-06-07 at 20:05 +0200, Rene Scharfe wrote: >> Argh, yes. Thanks, Pavel! > > Actually, thanks to Julian Seward for Valgrind. > >> However, the other branch is incorrect, too: >> accessing path->buf[path->len] is wrong, even if it's within the buffer. >> In order to use a length variable to point to the end of some string we >> need to subtract 1. *sigh* So, how about this one instead? > > Fine with me. Thank you for noticing! Thanks both. Applied. ```