Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve clipboard robustness #337

Merged
merged 7 commits into from
Sep 23, 2024
Merged

Conversation

layercak3
Copy link
Contributor

Some changes which improve our robustness with weird files or misbehaving clients.
I have read and understood CONTRIBUTING.md.

If a client/receiving Wayland clipboard client causes us to write() to a
closed pipe/socket, we'll get SIGPIPE which the default action is to
terminate the process.
Copy link
Owner

@any1 any1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

How did you test this?

src/data-control.c Outdated Show resolved Hide resolved
src/data-control.c Outdated Show resolved Hide resolved
src/data-control.c Outdated Show resolved Hide resolved
A misbehaving client can cause us to block when writing to the send fd,
such as if they create a pipe, read only a few bytes, then block
themselves, but we called write() with megabytes of data.

As for the receive fd, the Linux man page for select(2) says there may
be "circumstances in which a file descriptor is spuriously reported as
ready". This is probably never going to be a problem and we'd use epoll
on Linux anyway, but setting it doesn't hurt.

(O_NONBLOCK helps if the misbehaving client uses a pipe, but writing to
a disk would still "block" (more accurately, put us in non-interruptible
sleep). The protocol doesn't specify a limitation on what kinds of files
can be used, so there are probably various ways a client can freeze the
selection owner.)
This prevents sending incomplete data if the system or receiving Wayland
client causes partial writes to occur (especially now that we set
O_NONBLOCK). This needs to be asynchronous (not a while loop) to protect
us from misbehaving clients.

We only need to perform asynchronous sending if a partial write occurs
on the initial synchronous write. Otherwise, don't bother. That means we
should keep O_NONBLOCK for the initial synchronous write.
@layercak3
Copy link
Contributor Author

layercak3 commented Sep 23, 2024

How did you test this?

I used a small test client that sends a closed pipe (before patch, wayvnc gets SIGPIPE soon after the write call, but I guess has enough time to print that "write from clipboard incomplete" message), or does single a read() with a small buffer (before patch, wayvnc blocks on the write() call if its selection data is large)

client
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#include <wayland-client.h>
#include "wlr-data-control-unstable-v1-client-protocol.h"

static const char wayvnc_offer_prefix[] = "x-wayvnc-client-";

struct state {
        struct wl_display *display;
        struct wl_registry *registry;
        struct wl_seat *seat;
        struct zwlr_data_control_manager_v1 *data_control_manager;
        struct zwlr_data_control_device_v1 *data_control_device;
        struct zwlr_data_control_offer_v1 *current_offer;
};

static void bad_receive(struct state *state,
                struct zwlr_data_control_offer_v1 *offer)
{
        printf("Doing receive.\n");
        int pipe_fd[2];
        char *buf = malloc(8388608);
        ssize_t ret = 0;

        if (pipe(pipe_fd) == -1) {
                perror("pipe");
                return;
        }

        zwlr_data_control_offer_v1_receive(offer, "text/plain;charset=utf-8",
                        pipe_fd[1]);
        close(pipe_fd[1]);
//      close(pipe_fd[0]);

        wl_display_roundtrip(state->display);

/*      ret = read(pipe_fd[0], buf, 88);
        if (ret == -1) {
                perror("read");
                close(pipe_fd[0]);
        }*/

//      close(pipe_fd[0]);

        printf("Received bytes:\n%.*s\n", ret, buf);
}

static void handle_offer(void *data, struct zwlr_data_control_offer_v1 *offer,
                const char *mime_type)
{
        struct state *state = data;
        if (strncmp(wayvnc_offer_prefix, mime_type,
                                strlen(wayvnc_offer_prefix)) == 0)
                        state->current_offer = offer;
}

static const struct zwlr_data_control_offer_v1_listener data_control_offer_listener = {
        .offer = handle_offer,
};

static void handle_data_offer(void *data,
                struct zwlr_data_control_device_v1 *data_control_device,
                struct zwlr_data_control_offer_v1 *id)
{
        struct state *state = data;
        zwlr_data_control_offer_v1_add_listener(id, &data_control_offer_listener, data);
}

static void handle_selection(void *data,
                struct zwlr_data_control_device_v1 *data_control_device,
                struct zwlr_data_control_offer_v1 *id)
{
        struct state *state = data;

        if (!id) {
                if (state->current_offer) {
                        zwlr_data_control_offer_v1_destroy(state->current_offer);
                        state->current_offer = NULL;
                }
                return;
        }

        if (id == state->current_offer)
                bad_receive(state, id);

        zwlr_data_control_offer_v1_destroy(id);
        state->current_offer = NULL;
}

static void handle_finished(void *data,
                struct zwlr_data_control_device_v1 *zwlr_data_control_device_v1)
{
        struct state *state = data;
        zwlr_data_control_device_v1_destroy(zwlr_data_control_device_v1);
        if (state->data_control_device == zwlr_data_control_device_v1)
                state->data_control_device = NULL;
        if (state->current_offer) {
                zwlr_data_control_offer_v1_destroy(state->current_offer);
                state->current_offer = NULL;
        }
}

static void handle_primary_selection(void* data,
                struct zwlr_data_control_device_v1 *data_control_device,
                struct zwlr_data_control_offer_v1* id)
{
        struct state *state = data;

        if (!id) {
                if (state->current_offer) {
                        zwlr_data_control_offer_v1_destroy(state->current_offer);
                        state->current_offer = NULL;
                }
                return;
        }

        zwlr_data_control_offer_v1_destroy(id);
        state->current_offer = NULL;
}

static const struct zwlr_data_control_device_v1_listener data_control_device_listener = {
        .data_offer = handle_data_offer,
        .selection = handle_selection,
        .finished = handle_finished,
        .primary_selection = handle_primary_selection,
};

static void handle_global(void *data, struct wl_registry *registry,
                uint32_t name, const char *interface, uint32_t version)
{
        struct state *state = data;
        if (strcmp(interface, zwlr_data_control_manager_v1_interface.name) == 0) {
                state->data_control_manager = wl_registry_bind(registry, name,
                                &zwlr_data_control_manager_v1_interface, 2);
        } else if (strcmp(interface, wl_seat_interface.name) == 0) {
                state->seat = wl_registry_bind(registry, name,
                                &wl_seat_interface, 9);
        }
}

static void handle_global_remove(void *data, struct wl_registry *registry,
                uint32_t name)
{
}

static const struct wl_registry_listener registry_listener = {
        .global = handle_global,
        .global_remove = handle_global_remove,
};

int main()
{
        struct state state = {0};

        state.display = wl_display_connect(NULL);
        if (!state.display) {
                fprintf(stderr, "Failed to connect to the compositor.\n");
                return EXIT_FAILURE;
        }

        state.registry = wl_display_get_registry(state.display);
        wl_registry_add_listener(state.registry, &registry_listener, &state);

        wl_display_roundtrip(state.display);

        if (!state.data_control_manager) {
                fprintf(stderr, "This compositor doesn't support the wlr data control protocol.\n");
                return EXIT_FAILURE;
        }

        if (!state.seat) {
                fprintf(stderr, "The compositor didn't give us a seat.\n");
                return EXIT_FAILURE;
        }

        state.data_control_device = 
                zwlr_data_control_manager_v1_get_data_device(
                                state.data_control_manager, state.seat);
        zwlr_data_control_device_v1_add_listener(state.data_control_device,
                        &data_control_device_listener, &state);

        while (wl_display_dispatch(state.display) != -1) {}
}

For testing the patch I generated a large random text file and copied it on the client (to test the async send code): base64 /dev/urandom | head -c 8388607 > /tmp/data.txt; wl-copy < /tmp/data.txt
Launched tigervnc with -Log 'Viewport:stderr:100' -MaxCutText 8388608
Sometimes I have to hover over the window multiple times for tigervnc to actually send the large clipboard update. And often tigervnc will send a 0 byte clipboard update before sending the actual clipboard update. But these are just unrelated Xwayland issues.
Then when wayvnc became the selection owner I ran wl-paste on the server via ssh in a loop or launched a large number of background wl-paste jobs, and exited tigervnc (or terminated wl-paste) while the sends were taking place to test incomplete writes. (valgrind and writing megabytes of text to a terminal slows things down to make this easier)

One issue is that when running under valgrind and I run wl-paste & wl-paste & wl-paste & wl-paste & wl-paste & wl-paste & wl-paste & wl-paste & wl-paste & wl-paste & wl-paste (etc.) on the server via ssh while wayvnc is the selection owner with that large text, when I exit tigervnc while the sends are happening, the client is never unreffed/closed and the sends are never cancelled. After the sends finish the client is still never unreffed/closed. When not running in valgrind and I exit tigervnc these sends are always cancelled and the client closes properly.
Hmm, this isn't dependent on having a bunch of send contexts running. It happens sometimes when exiting tigervnc while a clipboard update is being transferred from the client to server. Anyway, this is likely caused by something somewhere else in the codebase or in neatvnc, maybe this.

@layercak3 layercak3 force-pushed the clipboard-robustness branch 3 times, most recently from 986fd24 to bcbfe79 Compare September 23, 2024 12:14
This was only necessary in the initial version of the file where offer
handling was synchronous.
@any1 any1 merged commit 5ae8265 into any1:master Sep 23, 2024
3 checks passed
@any1
Copy link
Owner

any1 commented Sep 23, 2024

Excellent. Thanks!

@layercak3 layercak3 deleted the clipboard-robustness branch September 24, 2024 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants