* Fixes for several memory leaks
@ 2011-04-02 20:18 cgit
2011-04-05 8:51 ` cgit
0 siblings, 1 reply; 5+ messages in thread
From: cgit @ 2011-04-02 20:18 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Fixes for several memory leaks
2011-04-02 20:18 Fixes for several memory leaks cgit
@ 2011-04-05 8:51 ` cgit
2011-04-07 10:15 ` hjemli
0 siblings, 1 reply; 5+ messages in thread
From: cgit @ 2011-04-05 8:51 UTC (permalink / raw)
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Fixes for several memory leaks
2011-04-05 8:51 ` cgit
@ 2011-04-07 10:15 ` hjemli
[not found] ` <20110407103025.GA20089@blizzard>
0 siblings, 1 reply; 5+ messages in thread
From: hjemli @ 2011-04-07 10:15 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Fixes for several memory leaks
[not found] ` <20110407103025.GA20089@blizzard>
@ 2011-04-07 10:38 ` hjemli
2011-04-10 17:31 ` cgit
0 siblings, 1 reply; 5+ messages in thread
From: hjemli @ 2011-04-07 10:38 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Fixes for several memory leaks
2011-04-07 10:38 ` hjemli
@ 2011-04-10 17:31 ` cgit
0 siblings, 0 replies; 5+ messages in thread
From: cgit @ 2011-04-10 17:31 UTC (permalink / raw)
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-04-10 17:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-02 20:18 Fixes for several memory leaks cgit
2011-04-05 8:51 ` cgit
2011-04-07 10:15 ` hjemli
[not found] ` <20110407103025.GA20089@blizzard>
2011-04-07 10:38 ` hjemli
2011-04-10 17:31 ` cgit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).