[PATCH weston,v2] refactor configuration API of fbdev-backend

[Pekka Paalanen]

On Tue, 26 Apr 2016 21:10:55 +0200
Benoit Gschwind <gschwind at gnu-log.net> wrote:

> Implement a "well" defined API to configure the fbdev backend.
> Following and according to discution about libweston API
> ---
> Signed-off-by: Benoit Gschwind <gschwind at gnu-log.net>

This S-o-b is on the wrong side of the '---'. git-am will drop if it is
below the '---'.

> 
> v1:
>  - add src/compositor-fbdev.h in the Makefile.am
>  - fix typo
> 
>  Makefile.am            |  3 ++-
>  src/compositor-fbdev.c | 45 +++++++++++++++++++++++----------------------
>  src/compositor-fbdev.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/main.c             | 33 +++++++++++++++++++++++++++++++--
>  4 files changed, 105 insertions(+), 25 deletions(-)
>  create mode 100644 src/compositor-fbdev.h

Hi Benoit,

would have been nice to rebase this on upstream master branch, but I
handled the conflicts for now to review it.

> 
> diff --git a/Makefile.am b/Makefile.am
> index c042c68..d319786 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -72,7 +72,8 @@ weston_SOURCES =					\
>  	src/log.c					\
>  	src/compositor.c				\
>  	src/compositor.h				\
> -	src/compositor-headless.h		\
> +	src/compositor-headless.h			\
> +	src/compositor-fbdev.h				\
>  	src/input.c					\
>  	src/data-device.c				\
>  	src/screenshooter.c				\

Makefile.am is still missing the other two places to add
compositor-fbdev.h to.

> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> index 19c5e3b..d0597a6 100644
> --- a/src/compositor-fbdev.c
> +++ b/src/compositor-fbdev.c
> @@ -44,6 +44,7 @@
>  
>  #include "shared/helpers.h"
>  #include "compositor.h"
> +#include "compositor-fbdev.h"
>  #include "launcher-util.h"
>  #include "pixman-renderer.h"
>  #include "libinput-seat.h"
> @@ -93,12 +94,6 @@ struct fbdev_output {
>  	uint8_t depth;
>  };
>  
> -struct fbdev_parameters {
> -	int tty;
> -	char *device;
> -	int use_gl;
> -};
> -
>  struct gl_renderer_interface *gl_renderer;
>  
>  static const char default_seat[] = "seat0";
> @@ -744,7 +739,7 @@ fbdev_restore(struct weston_compositor *compositor)
>  static struct fbdev_backend *
>  fbdev_backend_create(struct weston_compositor *compositor, int *argc, char *argv[],
>                       struct weston_config *config,
> -                     struct fbdev_parameters *param)
> +                     struct weston_fbdev_backend_config *param)
>  {
>  	struct fbdev_backend *backend;
>  	const char *seat_id = default_seat;
> @@ -827,29 +822,35 @@ out_compositor:
>  	return NULL;
>  }
>  
> +static void
> +config_init_to_defaults(struct weston_fbdev_backend_config *config)
> +{
> +	/* TODO: Ideally, available frame buffers should be enumerated using
> +	 * udev, rather than passing a device node in as a parameter. */
> +	config->tty = 0; /* default to current tty */
> +	config->device = "/dev/fb0"; /* default frame buffer */
> +	config->use_gl = 0;
> +}
> +
>  WL_EXPORT int
>  backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> -	     struct weston_config *config,
> +	     struct weston_config *wc,
>  	     struct weston_backend_config *config_base)
>  {
>  	struct fbdev_backend *b;
> -	/* TODO: Ideally, available frame buffers should be enumerated using
> -	 * udev, rather than passing a device node in as a parameter. */
> -	struct fbdev_parameters param = {
> -		.tty = 0, /* default to current tty */
> -		.device = "/dev/fb0", /* default frame buffer */
> -		.use_gl = 0,
> -	};
> +	struct weston_fbdev_backend_config config = {{ 0, }};
>  
> -	const struct weston_option fbdev_options[] = {
> -		{ WESTON_OPTION_INTEGER, "tty", 0, &param.tty },
> -		{ WESTON_OPTION_STRING, "device", 0, &param.device },
> -		{ WESTON_OPTION_BOOLEAN, "use-gl", 0, &param.use_gl },
> -	};
> +	if (config_base == NULL ||
> +	    config_base->struct_version != WESTON_FBDEV_BACKEND_CONFIG_VERSION ||
> +	    config_base->struct_size > sizeof(struct weston_fbdev_backend_config)) {
> +		weston_log("fbdev backend config structure is invalid\n");
> +		return -1;
> +	}
>  
> -	parse_options(fbdev_options, ARRAY_LENGTH(fbdev_options), argc, argv);
> +	config_init_to_defaults(&config);
> +	memcpy(&config, config_base, config_base->struct_size);
>  
> -	b = fbdev_backend_create(compositor, argc, argv, config, &param);
> +	b = fbdev_backend_create(compositor, argc, argv, wc, &config);

I am quoting the exact same comment I gave the last time:

> There is a small issue with config.device. It is ok for
> config_init_to_defaults() to use a statically allocated const string
> for it, because the pointer gets overwritten with a pointer from
> main.c. However, fbdev_output_create() is storing the pointer instead
> of calling strdup(). When main.c after init frees the pointer,
> fbdev_output::device will contain a stale pointer.
> 
> Could you send a preparation patch that makes fbdev_output_create() use
> strdup() for the device string, please?
> 
> I think it might have leaked the device string allocated by
> parse_options() before your patch.

What we talked about in IRC was the other comment, not this one.

Please, look again at the call chain after applying your patch:

main.c:
- load_fbdev_backend():
	The config.device string is malloc'd, and passed in to
	load_backend_new().

compositor-fbdev.c:
- backend_init(): Just copies the pointer, does not reallocate the string.
- fbdev_backend_create(): Calls fbdev_output_create().
- fbdev_output_create(): stores the string pointer in struct
  fbdev_output::device *without* copying.

main.c:
- load_fbdev_backend(): calls free() on the pointer.

This results in struct fbdev_output::device containing a pointer to
freed memory.

>  	if (b == NULL)
>  		return -1;
>  	return 0;


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160427/3884a393/attachment-0001.sig>