[3/3] revision: avoid leak when preparing bloom filter for "/"
diff mbox series

Message ID 20200804075017.GC284046@coredump.intra.peff.net
State New
Headers show
Series
  • some compiler/asan/ubsan fixes
Related show

Commit Message

Jeff King Aug. 4, 2020, 7:50 a.m. UTC
If we're given an empty pathspec, we refuse to set up bloom filters, as
described in f3c2a36810 (revision: empty pathspecs should not use Bloom
filters, 2020-07-01).

But before the empty string check, we drop any trailing slash by
allocating a new string without it. So a pathspec consisting only of "/"
will allocate that string, but then still cause us to bail, leaking the
new string. Let's make sure to free it.

Signed-off-by: Jeff King <peff@peff.net>
---
Just noticed while reading the function to fix the previous patch.

I'm not even sure if it's possible to get here with a pathspec of "/",
since we'd probably give a "/ is outside repository" error before then.

So maybe this case doesn't even matter. If it doesn't, then it might
simplify the function a bit to do the empty-pathspec check before
handling trailing slashes. But handling it does help make it more clear
this function is doing the right thing no matter what input it is given,
so that's what I went with here.

 revision.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Derrick Stolee Aug. 4, 2020, 1:09 p.m. UTC | #1
On 8/4/2020 3:50 AM, Jeff King wrote:
> If we're given an empty pathspec, we refuse to set up bloom filters, as
> described in f3c2a36810 (revision: empty pathspecs should not use Bloom
> filters, 2020-07-01).
> 
> But before the empty string check, we drop any trailing slash by
> allocating a new string without it. So a pathspec consisting only of "/"
> will allocate that string, but then still cause us to bail, leaking the
> new string. Let's make sure to free it.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Just noticed while reading the function to fix the previous patch.
> 
> I'm not even sure if it's possible to get here with a pathspec of "/",
> since we'd probably give a "/ is outside repository" error before then.
> 
> So maybe this case doesn't even matter. If it doesn't, then it might
> simplify the function a bit to do the empty-pathspec check before
> handling trailing slashes. But handling it does help make it more clear
> this function is doing the right thing no matter what input it is given,
> so that's what I went with here.

Works for me. Thanks for your careful attention here.

-Stolee

>  revision.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/revision.c b/revision.c
> index 5ed86e4524..b80868556b 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -702,6 +702,7 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)
>  	len = strlen(path);
>  	if (!len) {
>  		revs->bloom_filter_settings = NULL;
> +		free(path_alloc);
>  		return;
>  	}
>  
>
Taylor Blau Aug. 5, 2020, 3:19 p.m. UTC | #2
On Tue, Aug 04, 2020 at 09:09:21AM -0400, Derrick Stolee wrote:
> On 8/4/2020 3:50 AM, Jeff King wrote:
> > If we're given an empty pathspec, we refuse to set up bloom filters, as
> > described in f3c2a36810 (revision: empty pathspecs should not use Bloom
> > filters, 2020-07-01).
> >
> > But before the empty string check, we drop any trailing slash by
> > allocating a new string without it. So a pathspec consisting only of "/"
> > will allocate that string, but then still cause us to bail, leaking the
> > new string. Let's make sure to free it.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Just noticed while reading the function to fix the previous patch.
> >
> > I'm not even sure if it's possible to get here with a pathspec of "/",
> > since we'd probably give a "/ is outside repository" error before then.
> >
> > So maybe this case doesn't even matter. If it doesn't, then it might
> > simplify the function a bit to do the empty-pathspec check before

For what it's worth, I am almost certain that this isn't possible after
your last patch, but I also agree that it's not hurting anything in the
meantime, either. So...

> > handling trailing slashes. But handling it does help make it more clear
> > this function is doing the right thing no matter what input it is given,
> > so that's what I went with here.
>
> Works for me. Thanks for your careful attention here.

Works for me, too.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

> -Stolee
>
> >  revision.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/revision.c b/revision.c
> > index 5ed86e4524..b80868556b 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -702,6 +702,7 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)
> >  	len = strlen(path);
> >  	if (!len) {
> >  		revs->bloom_filter_settings = NULL;
> > +		free(path_alloc);
> >  		return;
> >  	}
> >
> >

Thanks,
Taylor

Patch
diff mbox series

diff --git a/revision.c b/revision.c
index 5ed86e4524..b80868556b 100644
--- a/revision.c
+++ b/revision.c
@@ -702,6 +702,7 @@  static void prepare_to_use_bloom_filter(struct rev_info *revs)
 	len = strlen(path);
 	if (!len) {
 		revs->bloom_filter_settings = NULL;
+		free(path_alloc);
 		return;
 	}