On 17 July 2014 19:54, Jasper St. Pierre wrote:
> The code here is wrong, leaky, and inconsistent. We don't free,
> unlink or clean up things when we should in every error path.
>
> Centralize the data destruction so it's easier to keep track of
> and easier to bug fix.
> ---
> src/wayland-server.c | 49
> -
> 1 file changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 1c9d4d0..3e0fb25 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -840,6 +840,23 @@ wl_display_create(void)
> return display;
> }
>
> +static void
> +wl_socket_destroy(struct wl_socket *s)
> +{
> + if (s->source)
> + wl_event_source_remove(s->source);
> + if (s->addr.sun_path[0])
> + unlink(s->addr.sun_path);
> + if (s->fd)
> + close(s->fd);
> + if (s->lock_addr[0])
> + unlink(s->lock_addr);
> + if (s->fd_lock)
> + close(s->fd_lock);
> +
> + free(s);
> +}
> +
> WL_EXPORT void
> wl_display_destroy(struct wl_display *display)
> {
> @@ -849,12 +866,7 @@ wl_display_destroy(struct wl_display *display)
> wl_signal_emit(&display->destroy_signal, display);
>
> wl_list_for_each_safe(s, next, &display->socket_list, link) {
> - wl_event_source_remove(s->source);
> - unlink(s->addr.sun_path);
> - close(s->fd);
> - unlink(s->lock_addr);
> - close(s->fd_lock);
> - free(s);
> + wl_socket_destroy(s);
> }
> wl_event_loop_destroy(display->loop);
>
> @@ -1072,7 +1084,7 @@ wl_display_add_socket(struct wl_display *display,
> const char *name)
>
> s->fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
> if (s->fd < 0) {
> - free(s);
> + wl_socket_destroy(s);
>
Here you are using uninitialized values, since malloc is not zeroing memory
and fd is the only filed set atm.
The same in the rest of wl_socket_destroy calls, until all fields are set.
Memset or calloc should fix it.
> return -1;
> }
>
> @@ -1090,8 +1102,7 @@ wl_display_add_socket(struct wl_display *display,
> const char *name)
> if (name_size > (int)sizeof s->addr.sun_path) {
> wl_log("error: socket path \"%s/%s\" plus null terminator"
>" exceeds 108 bytes\n", runtime_dir, name);
> - close(s->fd);
> - free(s);
> + wl_socket_destroy(s);
/* to prevent programs reporting
> * "failed to add socket: Success" */
> errno = ENAMETOOLONG;
> @@ -1100,28 +,20 @@ wl_display_add_socket(struct wl_display *display,
> const char *name)
>
> s->fd_lock = get_socket_lock(s);
> if (s->fd_lock < 0) {
> - close(s->fd);
> - free(s);
> + wl_socket_destroy(s);
> return -1;
> }
>
> size = offsetof (struct sockaddr_un, sun_path) + name_size;
> if (bind(s->fd, (struct sockaddr *) &s->addr, size) < 0) {
> wl_log("bind() failed with error: %m\n");
> - close(s->fd);
> - unlink(s->lock_addr);
> - close(s->fd_lock);
> - free(s);
> + wl_socket_destroy(s);
> return -1;
> }
>
> if (listen(s->fd, 1) < 0) {
> wl_log("listen() failed with error: %m\n");
> - unlink(s->addr.sun_path);
> - close(s->fd);
> - unlink(s->lock_addr);
> - close(s->fd_lock);
> - free(s);
> + wl_socket_destroy(s);
> return -1;
> }
>
> @@ -1129,11 +1132,7 @@ wl_display_add_socket(struct wl_display *display,
> const char *name)
> WL_EVENT_READABLE,
> socket_data, display);
> if (s->source == NULL) {
> - unlink(s->addr.sun_path);
> - close(s->fd);
> - unlink(s->lock_addr);
> - close(s->fd_lock);
> - free(s);
> + wl_socket_destroy(s);
> return -1;
> }
> wl_list_insert(display->socket_list.prev, &s->link);
> --
> 2.0.1
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
Otherwise looks OK.
Marek
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
