Fixes for several memory leaks

Fixes for several memory leaks

[cgit]

Hi,

I created patches for a whole bunch of memory leaks (probably around 100
leaks altogether) which mostly came up due to assigning newly allocated
strings (using xstrdupn() in most cases) to configuration strings
without free()'ing the previous pointer at all. Note that this doesn't
cover everything by far. There's a lot of more leaks (mainly with repo,
ref and commit handling) which I didn't have time to fix yet tho.

Please review and test in detail before pushing. I tried to cover
everything but I might have missed some tricky parts. I also only did
superficial tests...

Clone URL of my cgit working tree is [1], web interface of the "wip"
branch can be found on [2].

[1] git://git.cryptocrack.de/cgit.git
[2] http://git.cryptocrack.de/cgit.git/?h=wip

Fixes for several memory leaks

[cgit]

On Sat, Apr 02, 2011 at 10:18:44PM +0200, Lukas Fleischer wrote:
> Hi,
> 
> I created patches for a whole bunch of memory leaks (probably around 100
> leaks altogether) which mostly came up due to assigning newly allocated
> strings (using xstrdupn() in most cases) to configuration strings
> without free()'ing the previous pointer at all. Note that this doesn't
> cover everything by far. There's a lot of more leaks (mainly with repo,
> ref and commit handling) which I didn't have time to fix yet tho.
> 
> Please review and test in detail before pushing. I tried to cover
> everything but I might have missed some tricky parts. I also only did
> superficial tests...
> 
> Clone URL of my cgit working tree is [1], web interface of the "wip"
> branch can be found on [2].
> 
> [1] git://git.cryptocrack.de/cgit.git
> [2] http://git.cryptocrack.de/cgit.git/?h=wip

Added some null pointer dereference fixes.

Fixes for several memory leaks

[hjemli]

On Tue, Apr 5, 2011 at 10:51, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> On Sat, Apr 02, 2011 at 10:18:44PM +0200, Lukas Fleischer wrote:
>> I created patches for a whole bunch of memory leaks (probably around 100
>> leaks altogether) which mostly came up due to assigning newly allocated
>> strings (using xstrdupn() in most cases) to configuration strings
>> without free()'ing the previous pointer at all.

Thanks. Even though memleaks have been consciously ignored in cgit
(since it's a cgi app), I appreciate the cleanup which these paches
introduce.

>> Note that this doesn't
>> cover everything by far. There's a lot of more leaks (mainly with repo,
>> ref and commit handling) which I didn't have time to fix yet tho.

Well, it's not certain that we want to fix them...


>> Clone URL of my cgit working tree is [1], web interface of the "wip"
>> branch can be found on [2].
>>
>> [1] git://git.cryptocrack.de/cgit.git
>> [2] http://git.cryptocrack.de/cgit.git/?h=wip

The fix to trim_end() has a couple of alternate incarnations on the
list at the moment, we'll have to see which one ends up as preferred.

>
> Added some null pointer dereference fixes.

The fix to reencode() is probably a good safety measure for the future
(the bug cannot be triggered by current callers), but the right fix to
the issue found by clang-analyzer in cgit_print_diff() probably looks
like this:

+++ b/ui-diff.c
@@ -368,8 +368,10 @@ void cgit_print_diff(const char *new_rev, const char *old_r
                return;
        }
        commit = lookup_commit_reference(new_rev_sha1);
-       if (!commit || parse_commit(commit))
+       if (!commit || parse_commit(commit)) {
                cgit_print_error(fmt("Bad commit: %s", sha1_to_hex(new_rev_sha1)
+               return;
+       }

        if (old_rev)
                get_sha1(old_rev, old_rev_sha1);

--
larsh

Fixes for several memory leaks

[hjemli]

On Thu, Apr 7, 2011 at 12:30, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> On Thu, Apr 07, 2011 at 12:15:19PM +0200, Lars Hjemli wrote:
>> +++ b/ui-diff.c
>> @@ -368,8 +368,10 @@ void cgit_print_diff(const char *new_rev, const char *old_r
>> ? ? ? ? ? ? ? ? return;
>> ? ? ? ? }
>> ? ? ? ? commit = lookup_commit_reference(new_rev_sha1);
>> - ? ? ? if (!commit || parse_commit(commit))
>> + ? ? ? if (!commit || parse_commit(commit)) {
>> ? ? ? ? ? ? ? ? cgit_print_error(fmt("Bad commit: %s", sha1_to_hex(new_rev_sha1)
>> + ? ? ? ? ? ? ? return;
>> + ? ? ? }
>>
>> ? ? ? ? if (old_rev)
>> ? ? ? ? ? ? ? ? get_sha1(old_rev, old_rev_sha1);
>
> Yeah, I wasn't sure since there are other "Bad commit" error handlers
> below that do not "return;" as well, whereas all other error handlers
> do. My assumption that this is intended and execution should continue in
> this case (maybe to build the remaining page properly and display the
> error message somewhere inbetween).

No, the two missing returns (I only saw the first, thanks for
noticing) are simply bugs.

-- 
larsh

Fixes for several memory leaks

[cgit]

On Thu, Apr 07, 2011 at 12:38:50PM +0200, Lars Hjemli wrote:
> On Thu, Apr 7, 2011 at 12:30, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> > On Thu, Apr 07, 2011 at 12:15:19PM +0200, Lars Hjemli wrote:
> >> +++ b/ui-diff.c
> >> @@ -368,8 +368,10 @@ void cgit_print_diff(const char *new_rev, const char *old_r
> >> ? ? ? ? ? ? ? ? return;
> >> ? ? ? ? }
> >> ? ? ? ? commit = lookup_commit_reference(new_rev_sha1);
> >> - ? ? ? if (!commit || parse_commit(commit))
> >> + ? ? ? if (!commit || parse_commit(commit)) {
> >> ? ? ? ? ? ? ? ? cgit_print_error(fmt("Bad commit: %s", sha1_to_hex(new_rev_sha1)
> >> + ? ? ? ? ? ? ? return;
> >> + ? ? ? }
> >>
> >> ? ? ? ? if (old_rev)
> >> ? ? ? ? ? ? ? ? get_sha1(old_rev, old_rev_sha1);
> >
> > Yeah, I wasn't sure since there are other "Bad commit" error handlers
> > below that do not "return;" as well, whereas all other error handlers
> > do. My assumption that this is intended and execution should continue in
> > this case (maybe to build the remaining page properly and display the
> > error message somewhere inbetween).
> 
> No, the two missing returns (I only saw the first, thanks for
> noticing) are simply bugs.

Rebased my "wip" branch, changing the "Avoid null pointer dereference in
cgit_print_diff()." to what you proposed and removing the trim_end()
patch for now.