Re: column memory leak fix
> 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.
Re: column memory leak fix
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.)
Re: column memory leak fix
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.
Re: column memory leak fix
On Mon, Dec 30, 2013 at 10:32 PM, patrick keshishian wrote: > On Mon, Dec 30, 2013 at 04:58:50PM +0100, Mike Belopuhov wrote: >> On 30 December 2013 16:35, Loganaden Velvindron 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.
Re: column memory leak fix
Loganaden Velvindron 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.
Re: column memory leak fix
On Mon, Dec 30, 2013 at 04:58:50PM +0100, Mike Belopuhov wrote: > On 30 December 2013 16:35, Loganaden Velvindron 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 :-) > > >
Re: column memory leak fix
On 30 December 2013 16:35, Loganaden Velvindron 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 :-) >
Re: column memory leak fix
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 :-)
Re: column memory leak fix
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.
Re: column memory leak fix
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 - 1.16
> > > +++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -
> > > @@ -241,6 +241,9 @@ maketbl(void)
> > > (void)printf("%s\n", t->list[coloff]);
> > > }
> > > }
> > > + free(tbl);
> > > + free(cols);
> > > + free(lens);
> > > }
> > >
> > > #define DEFNUM 1000
> > >
Re: column memory leak fix
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 - 1.16
> > +++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -
> > @@ -241,6 +241,9 @@ maketbl(void)
> > (void)printf("%s\n", t->list[coloff]);
> > }
> > }
> > + free(tbl);
> > + free(cols);
> > + free(lens);
> > }
> >
> > #defineDEFNUM 1000
> >
Re: column memory leak fix
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 - 1.16
> +++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -
> @@ -241,6 +241,9 @@ maketbl(void)
> (void)printf("%s\n", t->list[coloff]);
> }
> }
> + free(tbl);
> + free(cols);
> + free(lens);
> }
>
> #define DEFNUM 1000
>
column memory leak fix
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 - 1.16
+++ src/usr.bin/column/column.c 30 Dec 2013 06:38:02 -
@@ -241,6 +241,9 @@ maketbl(void)
(void)printf("%s\n", t->list[coloff]);
}
}
+ free(tbl);
+ free(cols);
+ free(lens);
}
#defineDEFNUM 1000
