| Message ID | 20200804075017.GC284046@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 398e659e1ec60501d67a0f3cb1a1052c6e50038c |
| Headers | show |
| Series | some compiler/asan/ubsan fixes | expand |
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; > } > >
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
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; }
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(+)