Avoid allocating a big buffer for each loop iteration

There's no reason to allocate a big buffer for each loop iteration, only
to free it immediately afterwards.  Just allocate the buffer once and
reuse it.

This breaks the libqrexec ABI, but the changed functions are not used
outside of libqrexec itself.  To ensure that any problems are caught
early, move the functions to a separate header and mark them as having
hidden visibility.

Author: DemiMarie

fuzz/qrexec_remote_fuzzer.c

@@ -5,6 +5,7 @@
 #include <stdint.h>
 
 #include "libqrexec-utils.h"
+#include "remote.h"
 #include "fuzz.h"
 
 void _Noreturn fuzz_exit(int status) {
@@ -13,7 +14,17 @@ void _Noreturn fuzz_exit(int status) {
 
 void LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     fuzz_file_t *vchan_file, *stdin_file, *local_stderr_file;
-    struct buffer stdin_buf;
+    const size_t max_chunk_size = max_data_chunk_size(QREXEC_PROTOCOL_V2);
+    struct buffer stdin_buf  = {
+        .data = NULL,
+        .buflen = 0,
+    };
+    struct buffer remote_buf = {
+        .data = malloc(max_chunk_size),
+        .buflen = max_chunk_size,
+    };
+    if (remote_buf.data == NULL)
+        abort();
     int status;
 
     stdin_file = fuzz_file_create(0, NULL, 0);
@@ -23,14 +34,13 @@ void LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     stdin_file->open_read = false;
     local_stderr_file->open_read = false;
 
-    buffer_init(&stdin_buf);
-
-    handle_remote_data(
+    handle_remote_data_v2(
         vchan_file, stdin_file->fd, &status,
-        &stdin_buf, QREXEC_PROTOCOL_V2,
-        false, true, false);
+        &stdin_buf, false, true, (bool)false,
+        &remote_buf);
 
     fuzz_file_destroy(stdin_file);
     fuzz_file_destroy(vchan_file);
     fuzz_file_destroy(local_stderr_file);
+    free(remote_buf.data);
 }

libqrexec/libqrexec-utils.h

@@ -229,46 +229,11 @@ void do_replace_chars(char *buf, int len);
 #define REMOTE_EOF     0
 #define REMOTE_OK      1
 
-/*
- * Handle data from vchan. Sends MSG_DATA_STDIN and MSG_DATA_STDOUT to
- * specified FD (unless it's -1), and MSG_DATA_STDERR to our stderr.
- *
- * Return codes:
- *   REMOTE_EXITED - remote process terminated, do not send more data to it
- *     ("status" will be set)
- *   REMOTE_ERROR - vchan error occured
- *   REMOTE_EOF - EOF received, do not access this FD again
- *   REMOTE_OK - maybe some data processed, call again when buffer space and
- *     more data available
- *
- * Options:
- *   replace_chars_stdout, replace_chars_stderr - remove non-printable
- *     characters from stdout/stderr
- */
-int handle_remote_data(
-    libvchan_t *data_vchan, int stdin_fd, int *status,
-    struct buffer *stdin_buf, int data_protocol_version,
-    bool replace_chars_stdout, bool replace_chars_stderr, bool is_service);
-
 struct prefix_data {
     const char *data;
     size_t len;
 };
 
-/*
- * Handle data from the specified FD (cannot be -1) and send it over vchan
- * with a given message type (MSG_DATA_STDIN/STDOUT/STDERR).
- *
- * Return codes:
- *   REMOTE_ERROR - vchan error occured
- *   REMOTE_EOF - EOF received, do not access this FD again
- *   REMOTE_OK - some data processed, call it again when buffer space and
- *     more data availabla
- */
-int handle_input(
-    libvchan_t *vchan, int fd, int msg_type,
-    int data_protocol_version, struct prefix_data *data);
-
 int send_exit_code(libvchan_t *vchan, int status);
 
 /* Set of options for process_io(). */

libqrexec/process_io.c

@@ -30,6 +30,7 @@
 #include <unistd.h>
 
 #include "libqrexec-utils.h"
+#include "remote.h"
 
 static _Noreturn void handle_vchan_error(const char *op)
 {
@@ -107,8 +108,8 @@ int process_io(const struct process_io_request *req) {
     bool is_service = req->is_service;
     bool replace_chars_stdout = req->replace_chars_stdout;
     bool replace_chars_stderr = req->replace_chars_stderr;
-    int data_protocol_version = req->data_protocol_version;
-
+    const int data_protocol_version = req->data_protocol_version;
+    const size_t max_chunk_size = max_data_chunk_size(data_protocol_version);
     pid_t local_pid = req->local_pid;
     volatile sig_atomic_t *sigchld = req->sigchld;
     volatile sig_atomic_t *sigusr1 = req->sigusr1;
@@ -125,6 +126,19 @@ int process_io(const struct process_io_request *req) {
     struct timespec normal_timeout = { 10, 0 };
     struct prefix_data empty = { 0, 0 }, prefix = req->prefix_data;
 
+    struct buffer remote_buffer = {
+        .data = malloc(max_chunk_size),
+        .buflen = max_chunk_size,
+    };
+    if (remote_buffer.data == NULL)
+        handle_vchan_error("remote buffer alloc");
+    struct buffer stdin_buffer = {
+        .data = malloc(max_chunk_size),
+        .buflen = max_chunk_size,
+    };
+    if (stdin_buffer.data == NULL)
+        handle_vchan_error("stdin buffer alloc");
+
     sigemptyset(&pollmask);
     sigaddset(&pollmask, SIGCHLD);
     sigprocmask(SIG_BLOCK, &pollmask, NULL);
@@ -269,14 +283,14 @@ int process_io(const struct process_io_request *req) {
         }
 
         /* handle_remote_data will check if any data is available */
-        switch (handle_remote_data(
+        switch (handle_remote_data_v2(
                     vchan, stdin_fd,
                     &remote_status,
                     stdin_buf,
-                    data_protocol_version,
                     replace_chars_stdout > 0,
                     replace_chars_stderr > 0,
-                    is_service)) {
+                    is_service,
+                    &remote_buffer)) {
             case REMOTE_ERROR:
                 handle_vchan_error("read");
                 break;
@@ -296,9 +310,9 @@ int process_io(const struct process_io_request *req) {
                 break;
         }
         if (prefix.len > 0 || (stdout_fd >= 0 && fds[FD_STDOUT].revents)) {
-            switch (handle_input(
+            switch (handle_input_v2(
                         vchan, stdout_fd, stdout_msg_type,
-                        data_protocol_version, &prefix)) {
+                        &prefix, &stdin_buffer)) {
                 case REMOTE_ERROR:
                     handle_vchan_error("send(handle_input stdout)");
                     break;
@@ -309,9 +323,9 @@ int process_io(const struct process_io_request *req) {
             }
         }
         if (stderr_fd >= 0 && fds[FD_STDERR].revents) {
-            switch (handle_input(
+            switch (handle_input_v2(
                         vchan, stderr_fd, MSG_DATA_STDERR,
-                        data_protocol_version, &empty)) {
+                        &empty, &stdin_buffer)) {
                 case REMOTE_ERROR:
                     handle_vchan_error("send(handle_input stderr)");
                     break;
@@ -340,6 +354,9 @@ int process_io(const struct process_io_request *req) {
             PERROR("waitpid");
     }
 
+    free(remote_buffer.data);
+    free(stdin_buffer.data);
+
     if (!is_service && remote_status)
         return remote_status;
     return local_pid ? local_status : 0;

libqrexec/remote.c

@@ -30,18 +30,25 @@
 #include <assert.h>
 
 #include "libqrexec-utils.h"
+#include "remote.h"
 
-int handle_remote_data(
+int handle_remote_data_v2(
     libvchan_t *data_vchan, int stdin_fd, int *status,
-    struct buffer *stdin_buf, int data_protocol_version,
-    bool replace_chars_stdout, bool replace_chars_stderr, bool is_service)
+    struct buffer *stdin_buf,
+    bool replace_chars_stdout,
+    bool replace_chars_stderr,
+    bool is_service,
+    const struct buffer *buffer)
 {
     struct msg_header hdr;
-    const size_t max_len = max_data_chunk_size(data_protocol_version);
-    char *buf;
+    const size_t max_len = (size_t)buffer->buflen;
+    char *buf = buffer->data;
     int rc = REMOTE_ERROR;
     bool msg_data_warned = false;
 
+    if (buffer->buflen < 0)
+        abort();
+
     /* do not receive any data if we have something already buffered */
     switch (flush_client_data(stdin_fd, stdin_buf)) {
         case WRITE_STDIN_OK:
@@ -53,12 +60,6 @@ int handle_remote_data(
             return REMOTE_EOF;
     }
 
-    buf = malloc(max_len);
-    if (!buf) {
-        PERROR("malloc");
-        return REMOTE_ERROR;
-    }
-
     while (libvchan_data_ready(data_vchan) > 0) {
         if (libvchan_recv(data_vchan, &hdr, sizeof(hdr)) != sizeof(hdr))
             goto out;
@@ -143,25 +144,22 @@ int handle_remote_data(
     }
     rc = REMOTE_OK;
 out:
-    free(buf);
     return rc;
 }
 
-int handle_input(
+int handle_input_v2(
     libvchan_t *vchan, int fd, int msg_type,
-    int data_protocol_version, struct prefix_data *prefix_data)
+    struct prefix_data *prefix_data,
+    const struct buffer *buffer)
 {
-    const size_t max_len = max_data_chunk_size(data_protocol_version);
-    char *buf;
+    const size_t max_len = (size_t)buffer->buflen;
+    char *buf = buffer->data;
     ssize_t len;
     struct msg_header hdr;
     int rc = REMOTE_ERROR, buf_space;
 
-    buf = malloc(max_len);
-    if (!buf) {
-        PERROR("malloc");
-        return REMOTE_ERROR;
-    }
+    if (buffer->buflen < 0)
+        abort();
 
     static_assert(SSIZE_MAX >= INT_MAX, "can't happen on Linux");
     hdr.type = msg_type;
@@ -204,7 +202,6 @@ int handle_input(
     }
     rc = REMOTE_OK;
 out:
-    free(buf);
     return rc;
 }
 

libqrexec/remote.h

@@ -0,0 +1,64 @@
+/*
+ * The Qubes OS Project, http://www.qubes-os.org
+ *
+ * Copyright (C) 2010  Rafal Wojtczuk  <[email protected]>
+ * Copyright (C) 2013  Marek Marczykowski  <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#include <stdbool.h>
+#include <libvchan.h>
+
+#pragma GCC visibility push(hidden)
+
+/*
+ * Handle data from vchan. Sends MSG_DATA_STDIN and MSG_DATA_STDOUT to
+ * specified FD (unless it's -1), and MSG_DATA_STDERR to our stderr.
+ *
+ * Return codes:
+ *   REMOTE_EXITED - remote process terminated, do not send more data to it
+ *     ("status" will be set)
+ *   REMOTE_ERROR - vchan error occured
+ *   REMOTE_EOF - EOF received, do not access this FD again
+ *   REMOTE_OK - maybe some data processed, call again when buffer space and
+ *     more data available
+ *
+ * Options:
+ *   replace_chars_stdout, replace_chars_stderr - remove non-printable
+ *     characters from stdout/stderr
+ */
+int handle_remote_data_v2(
+    libvchan_t *data_vchan, int stdin_fd, int *status,
+    struct buffer *stdin_buf,
+    bool replace_chars_stdout, bool replace_chars_stderr, bool is_service,
+    const struct buffer *buffer);
+
+/*
+ * Handle data from the specified FD (cannot be -1) and send it over vchan
+ * with a given message type (MSG_DATA_STDIN/STDOUT/STDERR).
+ *
+ * Return codes:
+ *   REMOTE_ERROR - vchan error occured
+ *   REMOTE_EOF - EOF received, do not access this FD again
+ *   REMOTE_OK - some data processed, call it again when buffer space and
+ *     more data availabla
+ */
+int handle_input_v2(
+    libvchan_t *vchan, int fd, int msg_type,
+    struct prefix_data *prefix_data,
+    const struct buffer *buffer);
+#pragma GCC visibility pop