Re: [PATCH 07/26] http-backend: avoid memory leaks

[Johannes Schindelin]

Hi Junio,

On Sun, 30 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> Hmph.  I find a "leak" of a resource acquired inside the main
> >> function and not released when the main function leaves a lot less
> >> interesting than the other ones this series covers.
> >
> > Ah, I missed that this falls squarely into the "one-shot programs are
> > allowed to be sloppy in their memory management, essentially using
> > exit() as garbage collector" category.
> 
> I actually think it was a good intention of yours and I would agree
> with the patch.  The automated tools that find and complain about
> these issues do not know about our local house rules like "do not
> bother freeing immediately before exit" and will keep notifying us
> of leaks.

In Coverity, I marked them as "Intentional", so that they can be dropped
from the default view.

Unfortunately, the verdict "Intentional" does not percolate to other
projects, so I will probably have to do the entire shebang in the `git`
project again.

Of course, we could also decide that we want to shut up static analyzers
by actually *not* leaking memory, by *not* using exit() as our garbage
collector in one-shot commands, with the added benefit of making the code
easier to libify/reuse.

But I fear that I distract the discussion away from a more focused
attention to the actual patch series, which is actually still in flight
and in actual need for review, as I actually want to get those patches
integrated into git.git.

Ciao,
Dscho

Re: [PATCH 07/26] http-backend: avoid memory leaks

[Junio C Hamano]

Johannes Schindelin  writes:

>> Hmph.  I find a "leak" of a resource acquired inside the main
>> function and not released when the main function leaves a lot less
>> interesting than the other ones this series covers.
>
> Ah, I missed that this falls squarely into the "one-shot programs are
> allowed to be sloppy in their memory management, essentially using exit()
> as garbage collector" category.

I actually think it was a good intention of yours and I would agree
with the patch.  The automated tools that find and complain about
these issues do not know about our local house rules like "do not
bother freeing immediately before exit" and will keep notifying us
of leaks. One way to reduce the noise level so that we can focus on
real issues they may find for us in the future is to squelch these
"uninteresting" ones by "fixing" them, even if our local house rule
says OK, like this patch does.

I said "a lot less interesting" and did not mean "it is bad for our
code to do this".

Thanks.

Re: [PATCH 07/26] http-backend: avoid memory leaks

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Reported via Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  http-backend.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/http-backend.c b/http-backend.c
> > index eef0a361f4f..d12572fda10 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -681,8 +681,10 @@ int cmd_main(int argc, const char **argv)
> > if (!regexec(, dir, 1, out, 0)) {
> > size_t n;
> >  
> > -   if (strcmp(method, c->method))
> > +   if (strcmp(method, c->method)) {
> > +   free(dir);
> > return bad_request(, c);
> > +   }
> >  
> > cmd = c;
> > n = out[0].rm_eo - out[0].rm_so;
> > @@ -708,5 +710,7 @@ int cmd_main(int argc, const char **argv)
> >max_request_buffer);
> >  
> > cmd->imp(, cmd_arg);
> > +   free(dir);
> > +   free(cmd_arg);
> > return 0;
> >  }
> 
> Hmph.  I find a "leak" of a resource acquired inside the main
> function and not released when the main function leaves a lot less
> interesting than the other ones this series covers.

Ah, I missed that this falls squarely into the "one-shot programs are
allowed to be sloppy in their memory management, essentially using exit()
as garbage collector" category.

Will drop,
Dscho

Re: [PATCH 07/26] http-backend: avoid memory leaks

[Junio C Hamano]

Johannes Schindelin  writes:

> Reported via Coverity.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  http-backend.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/http-backend.c b/http-backend.c
> index eef0a361f4f..d12572fda10 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -681,8 +681,10 @@ int cmd_main(int argc, const char **argv)
>   if (!regexec(, dir, 1, out, 0)) {
>   size_t n;
>  
> - if (strcmp(method, c->method))
> + if (strcmp(method, c->method)) {
> + free(dir);
>   return bad_request(, c);
> + }
>  
>   cmd = c;
>   n = out[0].rm_eo - out[0].rm_so;
> @@ -708,5 +710,7 @@ int cmd_main(int argc, const char **argv)
>  max_request_buffer);
>  
>   cmd->imp(, cmd_arg);
> + free(dir);
> + free(cmd_arg);
>   return 0;
>  }

Hmph.  I find a "leak" of a resource acquired inside the main
function and not released when the main function leaves a lot less
interesting than the other ones this series covers.

When the "return" in the first hunk leaves the function and also
the same block of the if () statement breaks out the loop, we also
fail to call regfree().  Shouldn't we be cleaning it up as well?

[PATCH 07/26] http-backend: avoid memory leaks

[Johannes Schindelin]

Reported via Coverity.

Signed-off-by: Johannes Schindelin 
---
 http-backend.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/http-backend.c b/http-backend.c
index eef0a361f4f..d12572fda10 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -681,8 +681,10 @@ int cmd_main(int argc, const char **argv)
if (!regexec(, dir, 1, out, 0)) {
size_t n;
 
-   if (strcmp(method, c->method))
+   if (strcmp(method, c->method)) {
+   free(dir);
return bad_request(, c);
+   }
 
cmd = c;
n = out[0].rm_eo - out[0].rm_so;
@@ -708,5 +710,7 @@ int cmd_main(int argc, const char **argv)
   max_request_buffer);
 
cmd->imp(, cmd_arg);
+   free(dir);
+   free(cmd_arg);
return 0;
 }
-- 
2.12.2.windows.2.800.gede8f145e06