[PATCH weston] compositor.c: do not leak option strings

[Ryo Munakata]

On Fri, 5 Sep 2014 13:56:23 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> > +		if (!backend) {
> > +			if (getenv("WAYLAND_DISPLAY") || getenv("WAYLAND_SOCKET"))
> > +				backend = strdup("wayland-backend.so");
> > +			else if (getenv("DISPLAY"))
> > +				backend = strdup("x11-backend.so");
> > +			else
> > +				backend = strdup(WESTON_NATIVE_BACKEND);
> > +		}
> 
> While working here, I think this piece of code could be extracted to a
> new function weston_choose_default_backend(). The extraction could be
> the first patch in the series.
> 
> We might want to make the default backend choosing a bit smarter in the
> future, like not attempting to use backends we did not build.
> 
> >  	}
> >  
> >  	backend_init = weston_load_module(backend, "backend_init");
> > @@ -4440,24 +4437,26 @@ int main(int argc, char *argv[])
> >  		if (socket_name) {
> >  			if (wl_display_add_socket(display, socket_name)) {
> >  				weston_log("fatal: failed to add socket: %m\n");
> > -				ret = EXIT_FAILURE;
> > -				goto out;
> > +				free(socket_name);
> > +				socket_name = NULL;
> >  			}
> >  		} else {
> >  			socket_name = wl_display_add_socket_auto(display);
> > -			if (!socket_name) {
> > +			if (socket_name)
> > +				socket_name = strdup(socket_name);
> > +			else
> >  				weston_log("fatal: failed to add socket: %m\n");
> > -				ret = EXIT_FAILURE;
> > -				goto out;
> > -			}
> >  		}
> 
> I think we could extract another function here, that would include
> everything inside the else-clause of if (fd != -1), and call
> it e.g. int weston_create_listening_socket(display, socket_name).
> No need to return the final socket_name to main(), I believe.
> 
> Then in main() you could just unconditionally call free(socket_name)
> once, since free(NULL) is ok.
> 
> >  
> > +		if (!socket_name) {
> > +			ret = EXIT_FAILURE;
> > +			goto out;
> > +		}
> >  		setenv("WAYLAND_DISPLAY", socket_name, 1);
> > +		free(socket_name);
> >  	}
> >  
> > -	if (option_shell)
> > -		shell = strdup(option_shell);
> > -	else
> > +	if (!shell)
> >  		weston_config_section_get_string(section, "shell", &shell,
> >  						 "desktop-shell.so");
> >  
> > @@ -4474,8 +4473,11 @@ int main(int argc, char *argv[])
> >  	}
> >  	free(modules);
> >  
> > -	if (load_modules(ec, option_modules, &argc, argv) < 0)
> > +	if (load_modules(ec, option_modules, &argc, argv) < 0) {
> > +		free(option_modules);
> >  		goto out;
> > +	}
> > +	free(option_modules);
> >  
> >  	weston_compositor_wake(ec);
> >  
> 
> You could just have one free(option_modules) somewhere after out: label.
> 
> We could collect all free()s for the WESTON_OPTION_STRING variables at
> the end of main().
> 
> 
> Thanks,
> pq

Hi, Pekka.

I sent a series of the version 2 of this patch, which fixes the issues you mentioned.

I worried about the usage of not-free'd memory caused by collecting free()s at the end of main().
But it will be very small and now I think it doesn't matter.

Thanks.
-- 
Ryo Munakata <ryomnktml at gmail.com>