column memory leak fix

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

column memory leak fix

Loganaden Velvindron
Hi All,

From NetBSD:

Plug memory leak. Coverity CID 1596

Index: src/usr.bin/column/column.c
===================================================================
RCS file: /cvs/src/usr.bin/column/column.c,v
retrieving revision 1.16
diff -u -p -r1.16 column.c
--- src/usr.bin/column/column.c 26 Nov 2013 13:18:55 -0000 1.16
+++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -0000
@@ -241,6 +241,9 @@ maketbl(void)
  (void)printf("%s\n", t->list[coloff]);
  }
  }
+ free(tbl);
+ free(cols);
+ free(lens);
 }
 
 #define DEFNUM 1000

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Mike Belopuhov-5
On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
> Hi All,
>
> From NetBSD:
>
> Plug memory leak. Coverity CID 1596
>

memory leak?  can you please elaborate where else this memory
is "leaking" if not back to the system.

> Index: src/usr.bin/column/column.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/column/column.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 column.c
> --- src/usr.bin/column/column.c 26 Nov 2013 13:18:55 -0000 1.16
> +++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -0000
> @@ -241,6 +241,9 @@ maketbl(void)
>   (void)printf("%s\n", t->list[coloff]);
>   }
>   }
> + free(tbl);
> + free(cols);
> + free(lens);
>  }
>  
>  #define DEFNUM 1000
>

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Loganaden Velvindron
On Mon, Dec 30, 2013 at 12:45:47PM +0100, Mike Belopuhov wrote:

> On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
> > Hi All,
> >
> > From NetBSD:
> >
> > Plug memory leak. Coverity CID 1596
> >
>
> memory leak?  can you please elaborate where else this memory
> is "leaking" if not back to the system.

After a short discussion on IRC, and some digging, it makes sense
that the system cleanups up automatically, and therefore it is not
necessary.


>
> > Index: src/usr.bin/column/column.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/column/column.c,v
> > retrieving revision 1.16
> > diff -u -p -r1.16 column.c
> > --- src/usr.bin/column/column.c 26 Nov 2013 13:18:55 -0000 1.16
> > +++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -0000
> > @@ -241,6 +241,9 @@ maketbl(void)
> >   (void)printf("%s\n", t->list[coloff]);
> >   }
> >   }
> > + free(tbl);
> > + free(cols);
> > + free(lens);
> >  }
> >  
> >  #define DEFNUM 1000
> >

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Mike Belopuhov-5
On Mon, Dec 30, 2013 at 03:59 -0800, Loganaden Velvindron wrote:

> On Mon, Dec 30, 2013 at 12:45:47PM +0100, Mike Belopuhov wrote:
> > On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
> > > Hi All,
> > >
> > > From NetBSD:
> > >
> > > Plug memory leak. Coverity CID 1596
> > >
> >
> > memory leak?  can you please elaborate where else this memory
> > is "leaking" if not back to the system.
>
> After a short discussion on IRC, and some digging, it makes sense
> that the system cleanups up automatically, and therefore it is not
> necessary.
>

the patch can be committed though for the sake of better style.

>
> >
> > > Index: src/usr.bin/column/column.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.bin/column/column.c,v
> > > retrieving revision 1.16
> > > diff -u -p -r1.16 column.c
> > > --- src/usr.bin/column/column.c 26 Nov 2013 13:18:55 -0000 1.16
> > > +++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -0000
> > > @@ -241,6 +241,9 @@ maketbl(void)
> > >   (void)printf("%s\n", t->list[coloff]);
> > >   }
> > >   }
> > > + free(tbl);
> > > + free(cols);
> > > + free(lens);
> > >  }
> > >  
> > >  #define DEFNUM 1000
> > >

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Ted Unangst-6
In reply to this post by Mike Belopuhov-5
On Mon, Dec 30, 2013 at 13:53, Mike Belopuhov wrote:

> On Mon, Dec 30, 2013 at 03:59 -0800, Loganaden Velvindron wrote:
>> On Mon, Dec 30, 2013 at 12:45:47PM +0100, Mike Belopuhov wrote:
>> > On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
>> > > Hi All,
>> > >
>> > > From NetBSD:
>> > >
>> > > Plug memory leak. Coverity CID 1596
>> > >
>> >
>> > memory leak?  can you please elaborate where else this memory
>> > is "leaking" if not back to the system.
>>
>> After a short discussion on IRC, and some digging, it makes sense
>> that the system cleanups up automatically, and therefore it is not
>> necessary.
>>
>
> the patch can be committed though for the sake of better style.

but please review to make sure they are correct.

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Loganaden Velvindron
On Mon, Dec 30, 2013 at 08:42:00AM -0500, Ted Unangst wrote:

> On Mon, Dec 30, 2013 at 13:53, Mike Belopuhov wrote:
> > On Mon, Dec 30, 2013 at 03:59 -0800, Loganaden Velvindron wrote:
> >> On Mon, Dec 30, 2013 at 12:45:47PM +0100, Mike Belopuhov wrote:
> >> > On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
> >> > > Hi All,
> >> > >
> >> > > From NetBSD:
> >> > >
> >> > > Plug memory leak. Coverity CID 1596
> >> > >
> >> >
> >> > memory leak?  can you please elaborate where else this memory
> >> > is "leaking" if not back to the system.
> >>
> >> After a short discussion on IRC, and some digging, it makes sense
> >> that the system cleanups up automatically, and therefore it is not
> >> necessary.
> >>
> >
> > the patch can be committed though for the sake of better style.
>
> but please review to make sure they are correct.

It would be great if the ldconfig, and the various maintainers could
review the diff, and help me improve the diffs, or discard them.

I'm mostly interested in finding the small security issues and fixing
them, rather than fixing style issues :-)

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Mike Belopuhov-5
On 30 December 2013 16:35, Loganaden Velvindron <[hidden email]> wrote:

> On Mon, Dec 30, 2013 at 08:42:00AM -0500, Ted Unangst wrote:
>> On Mon, Dec 30, 2013 at 13:53, Mike Belopuhov wrote:
>> > On Mon, Dec 30, 2013 at 03:59 -0800, Loganaden Velvindron wrote:
>> >> On Mon, Dec 30, 2013 at 12:45:47PM +0100, Mike Belopuhov wrote:
>> >> > On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
>> >> > > Hi All,
>> >> > >
>> >> > > From NetBSD:
>> >> > >
>> >> > > Plug memory leak. Coverity CID 1596
>> >> > >
>> >> >
>> >> > memory leak?  can you please elaborate where else this memory
>> >> > is "leaking" if not back to the system.
>> >>
>> >> After a short discussion on IRC, and some digging, it makes sense
>> >> that the system cleanups up automatically, and therefore it is not
>> >> necessary.
>> >>
>> >
>> > the patch can be committed though for the sake of better style.
>>
>> but please review to make sure they are correct.
>
> It would be great if the ldconfig, and the various maintainers could
> review the diff, and help me improve the diffs, or discard them.
>

but the same applies to the ldconfig "fix".  buildhints is called right
before exit.  there are no leaks there.  the only thing that can be
improved is unlinking tmpfilenam.

> I'm mostly interested in finding the small security issues and fixing
> them, rather than fixing style issues :-)
>

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

patrick keshishian-2
On Mon, Dec 30, 2013 at 04:58:50PM +0100, Mike Belopuhov wrote:

> On 30 December 2013 16:35, Loganaden Velvindron <[hidden email]> wrote:
> > On Mon, Dec 30, 2013 at 08:42:00AM -0500, Ted Unangst wrote:
> >> On Mon, Dec 30, 2013 at 13:53, Mike Belopuhov wrote:
> >> > On Mon, Dec 30, 2013 at 03:59 -0800, Loganaden Velvindron wrote:
> >> >> On Mon, Dec 30, 2013 at 12:45:47PM +0100, Mike Belopuhov wrote:
> >> >> > On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
> >> >> > > Hi All,
> >> >> > >
> >> >> > > From NetBSD:
> >> >> > >
> >> >> > > Plug memory leak. Coverity CID 1596
> >> >> > >
> >> >> >
> >> >> > memory leak?  can you please elaborate where else this memory
> >> >> > is "leaking" if not back to the system.
> >> >>
> >> >> After a short discussion on IRC, and some digging, it makes sense
> >> >> that the system cleanups up automatically, and therefore it is not
> >> >> necessary.
> >> >>
> >> >
> >> > the patch can be committed though for the sake of better style.
> >>
> >> but please review to make sure they are correct.
> >
> > It would be great if the ldconfig, and the various maintainers could
> > review the diff, and help me improve the diffs, or discard them.
> >
>
> but the same applies to the ldconfig "fix".  buildhints is called right
> before exit.  there are no leaks there.  the only thing that can be
> improved is unlinking tmpfilenam.

Maybe so, but ever since I've known of OpenBSD, it has always
"preached" good coding practices and exemplified through its
base source code. So why are you discouraging such fixes? You
are almost saying that any non-daemon program should not bother
with free(), close() and similar resource de-allocation function,
because the system will do the reclaiming after program exits.
That's rubbish.

Regards,
--patrick

> > I'm mostly interested in finding the small security issues and fixing
> > them, rather than fixing style issues :-)
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Alexander Hall
In reply to this post by Loganaden Velvindron


Loganaden Velvindron <[hidden email]> wrote:

>I'm mostly interested in finding the small security issues and fixing
>them, rather than fixing style issues :-)

Keeping good style helps avoiding bugs, though.

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Loganaden Velvindron-2
In reply to this post by patrick keshishian-2
On Mon, Dec 30, 2013 at 10:32 PM, patrick keshishian
<[hidden email]> wrote:

> On Mon, Dec 30, 2013 at 04:58:50PM +0100, Mike Belopuhov wrote:
>> On 30 December 2013 16:35, Loganaden Velvindron <[hidden email]> wrote:
>> > On Mon, Dec 30, 2013 at 08:42:00AM -0500, Ted Unangst wrote:
>> >> On Mon, Dec 30, 2013 at 13:53, Mike Belopuhov wrote:
>> >> > On Mon, Dec 30, 2013 at 03:59 -0800, Loganaden Velvindron wrote:
>> >> >> On Mon, Dec 30, 2013 at 12:45:47PM +0100, Mike Belopuhov wrote:
>> >> >> > On Sun, Dec 29, 2013 at 22:45 -0800, Loganaden Velvindron wrote:
>> >> >> > > Hi All,
>> >> >> > >
>> >> >> > > From NetBSD:
>> >> >> > >
>> >> >> > > Plug memory leak. Coverity CID 1596
>> >> >> > >
>> >> >> >
>> >> >> > memory leak?  can you please elaborate where else this memory
>> >> >> > is "leaking" if not back to the system.
>> >> >>
>> >> >> After a short discussion on IRC, and some digging, it makes sense
>> >> >> that the system cleanups up automatically, and therefore it is not
>> >> >> necessary.
>> >> >>
>> >> >
>> >> > the patch can be committed though for the sake of better style.
>> >>
>> >> but please review to make sure they are correct.
>> >
>> > It would be great if the ldconfig, and the various maintainers could
>> > review the diff, and help me improve the diffs, or discard them.
>> >
>>
>> but the same applies to the ldconfig "fix".  buildhints is called right
>> before exit.  there are no leaks there.  the only thing that can be
>> improved is unlinking tmpfilenam.
>
> Maybe so, but ever since I've known of OpenBSD, it has always
> "preached" good coding practices and exemplified through its
> base source code. So why are you discouraging such fixes? You
> are almost saying that any non-daemon program should not bother
> with free(), close() and similar resource de-allocation function,
> because the system will do the reclaiming after program exits.
> That's rubbish.

Hi Patrick, the base source code is pretty huge, and I'm sure that there are
memory/fd leak issues lurking around that need to be fixed. I'd much rather
spend time finding and fixing them :-)

After I got feedback on my diffs, I looked at another base program (user(8)) and
asked jj@ to have a look at the diff I just sent.

Let the developers decide what they think is the right way. Until they
reach consensus,
we can use our limited time to fix maximum number of bugs for OpenBSD
5.5 release :-)


>
> Regards,
> --patrick
>
>> > I'm mostly interested in finding the small security issues and fixing
>> > them, rather than fixing style issues :-)
>> >
>>
>



--
This message is strictly personal and the opinions expressed do not
represent those of my employers, either past or present.

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Ted Unangst-6
In reply to this post by Loganaden Velvindron
On Mon, Dec 30, 2013 at 10:32, patrick keshishian wrote:
> Maybe so, but ever since I've known of OpenBSD, it has always
> "preached" good coding practices and exemplified through its
> base source code. So why are you discouraging such fixes? You

My concern is that these patches are being taken from netbsd without
consideration for whether they are the best fix for openbsd, or even
correct. That concern is compounded by my sense that the netbsd
developers are robotically making changes to appease tools without
testing them.

I'm a big fan of static analysis and I spent several years working at
Coverity, but it is a dangerous tool in the wrong hands.

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Ted Unangst-6
In reply to this post by Mike Belopuhov-5
On Mon, Dec 30, 2013 at 22:50, Loganaden Velvindron wrote:

> Hi Patrick, the base source code is pretty huge, and I'm sure that there are
> memory/fd leak issues lurking around that need to be fixed. I'd much rather
> spend time finding and fixing them :-)
>
> After I got feedback on my diffs, I looked at another base program
> (user(8)) and
> asked jj@ to have a look at the diff I just sent.

The resource handling in user is a mess, I'll give you that. It also
does some unnecessary freeing, such as immediately before calling
errx().

I think I would prefer to see a more comprehensive diff, though,
rather than trying to cherry pick changes from netbsd. Read
through the source, find the bugs, and think about corrections.
Improve the code instead of just tweaking it.

(There are plenty of times when small tweaks are better. So far,
though, I think we should consider these small bugs as an opportunity
for a more comprehensive review of each program.)

Reply | Threaded
Open this post in threaded view
|

Re: column memory leak fix

Theo de Raadt
In reply to this post by Loganaden Velvindron
> I'm mostly interested in finding the small security issues and fixing
> them, rather than fixing style issues :-)

As yet, none of these have anything to do with security.

Many of the rest are about to get cleaned up by exit, so they are not
really fixes of value either.

So they come down to style.