Re: [Nbd] [PATCH 4/4] nbd-server: remove global variables: modern_listen and modernport

[Tuomas Jorma Juhani Räsänen <tuomasjjrasanen@...1261...>]

On Fri, Jan 04, 2013 at 04:12:43PM +0100, Wouter Verhelst wrote:
> On Fri, Jan 04, 2013 at 03:10:09PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> > On Fri, Jan 04, 2013 at 02:40:59AM +0100, Wouter Verhelst wrote:
> > > On Thu, Jan 03, 2013 at 11:21:16PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> > > > @@ -2752,11 +2748,13 @@ int main(int argc, char *argv[]) {
> > > >  	}
> > > >  	if (!dontfork)
> > > >  		daemonize(serve);
> > > > -	setup_servers(servers);
> > > > +	setup_servers(servers, genconf.modernaddr, genconf.modernport);
> > > >  	dousers(genconf.user, genconf.group);
> > > >  
> > > >          g_free(genconf.user);
> > > >          g_free(genconf.group);
> > > > +        g_free(genconf.modernaddr);
> > > > +        g_free(genconf.modernport);
> > > 
> > > same here: why?
> > 
> > The same reasoning as in the previous patch: genconf.modernaddr and
> > genconf.modernport are allocated by g_key_file_get_string() and needs
> > to be freed at some point. After this point, no one cares about them
> > anymore, so it's safe to free them.
> 
> Mmm. My point (in both cases) was more that just because you don't need
> something anymore shouldn't mean you need to also throw it out. For
> instance, if we ever want to go and do a second parsing of the config
> file (which is what you're trying to do here, right?) then it may make
> sense to be able to error out on "the username was changed". It's hard
> to do that if you don't have the user name anymore because you already
> free()d the string.
> 
> Yes, technically it's a leak if you don't release memory until program
> end, but I've never considered one-time leaks a problem; due to
> fragmentation, you most likely won't be reusing that memory anyway, so
> there's not much of a benefit here; and if we have a non-NULL pointer in
> the "user we're running as" variable, then we can use that fact to
> signal whether we did in fact use that option at some later point in the
> program, should we ever need to.
> 
> If your concern is that we'll need to have it freed when rereading the
> config file, it's not too late to free it after comparing the old values
> with the new ones...
> 

Ah, I see your point now. You are looking a bit further than I was
with this patch..

Yes, one of my goals was to make it possible to read configuration
multiple times without breaking anything. Then another goal is to do
something useful with it, like we have been discussing earlier,
parsing configuration on SIGHUP and then updating the runtime behavior
accordingly. That patch is still on my workbench. And it will be there
until all its requirements (re-read conf, error handling) are
fulfilled.

So, when writing this (or the other) patch, I didn't want to think too
much how the username or the groupname could be exploited in future. I
just tried to create a patch which would be correct on its own without
any requirements for the future.

Then the patch, which compares an old username and a new username and
does something neat with that information, would be responsible of
removing these free()s.

To summarize, the reason behind those free()s was to make the patch
correct. But, like you said, the leak is insignificant in practice so
it wouldn't be too wrong to leave those out. It's your call =).

-- 
Tuomas