[John Thacker <johnthacker@xxxxxxxxx>]

MR from patch:

https://gitlab.com/wireshark/wireshark/-/merge_requests/16383

I already submitted a patch for closing the pipe, which should help with error reports. Of course it would be better to be able to deal with multiple reads.

Thanks again,

John

On Wed, Jul 10, 2024 at 8:12 PM John Thacker <johnthacker@xxxxxxxxx> wrote:

Yes, the buffer needs to be declared on the heap to make it larger. There's also an assertion that isn't in effect when the code is optimized.

It's relatively simple to fix the underlying issue- in sync_interface_stats_open the pipe is closed on an error, but it should be closed before waiting on the child, not after.

Thanks for the report.

On Wed, Jul 10, 2024 at 7:38 PM <greearb@xxxxxxxxxxxxxxx> wrote:

From: Ben Greear <greearb@xxxxxxxxxxxxxxx>

This code is still fragile, it should not deadlock even if there
is too much to read, but this at least hacks around the problem.

To avoid huge stacks, allocate the buffer on the heap.

Fixes wireshark hang with about 50 interfaces...

Signed-off-by: Ben Greear <greearb@xxxxxxxxxxxxxxx>
---
 capture/capture_sync.c | 18 +++++++++++++++---
 sync_pipe.h            |  2 +-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/capture/capture_sync.c b/capture/capture_sync.c
index 09cdb17c3b..8e1a1d9b52 100644
--- a/capture/capture_sync.c
+++ b/capture/capture_sync.c
@@ -1034,7 +1034,7 @@ sync_pipe_run_command_actual(char **argv, char **data, char **primary_msg,
     GIOChannel *sync_pipe_read_io;
     ws_process_id fork_child;
     char *wait_msg;
-    char buffer[PIPE_BUF_SIZE+1] = {0};
+    char *buffer = malloc(PIPE_BUF_SIZE + 1);
     ssize_t nread;
     char indicator;
     int32_t exec_errno = 0;
@@ -1052,6 +1052,7 @@ sync_pipe_run_command_actual(char **argv, char **data, char **primary_msg,
         *primary_msg = msg;
         *secondary_msg = NULL;
         *data = ""> +        free(buffer);
         return -1;
     }

@@ -1096,6 +1097,7 @@ sync_pipe_run_command_actual(char **argv, char **data, char **primary_msg,
             }
             *secondary_msg = NULL;
             *data = ""> +            free(buffer);

             return -1;
         }
@@ -1241,6 +1243,7 @@ sync_pipe_run_command_actual(char **argv, char **data, char **primary_msg,
         }
     } while (indicator != SP_SUCCESS && ret != -1);

+    free(buffer);
     return ret;
 }

@@ -1470,7 +1473,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **d
     int ret;
     GIOChannel *message_read_io;
     char *wait_msg;
-    char buffer[PIPE_BUF_SIZE+1] = {0};
+    char *buffer = malloc(PIPE_BUF_SIZE + 1);
     ssize_t nread;
     char indicator;
     int32_t exec_errno = 0;
@@ -1486,6 +1489,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **d

     if (!argv) {
         *msg = g_strdup("We don't know where to find dumpcap.");
+        free(buffer);
         return -1;
     }

@@ -1503,6 +1507,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **d
     argv = sync_pipe_add_arg(argv, &argc, "--signal-pipe");
     ret = create_dummy_signal_pipe(msg);
     if (ret == -1) {
+        free(buffer);
         return -1;
     }
     argv = sync_pipe_add_arg(argv, &argc, dummy_control_id);
@@ -1511,6 +1516,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **d
     ret = sync_pipe_open_command(argv, data_read_fd, &message_read_io, NULL,
                                  fork_child, NULL, msg, update_cb);
     if (ret == -1) {
+        free(buffer);
         return -1;
     }

@@ -1555,6 +1561,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **d
                     *msg = combined_msg;
                 }
             }
+            free(buffer);
             return -1;
         }

@@ -1625,6 +1632,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **d
                 *msg = g_strdup(primary_msg_text);
                 ret = -1;
             }
+            free(buffer);
             return ret;

         case SP_LOG_MSG:
@@ -1673,6 +1681,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **d
         }
     } while (indicator != SP_SUCCESS && ret != -1);

+    free(buffer);
     return ret;
 }

@@ -1859,7 +1868,7 @@ static gboolean
 sync_pipe_input_cb(GIOChannel *pipe_io, capture_session *cap_session)
 {
     int  ret;
-    char buffer[SP_MAX_MSG_LEN+1] = {0};
+    char *buffer = malloc(SP_MAX_MSG_LEN + 1);
     ssize_t nread;
     char indicator;
     int32_t exec_errno = 0;
@@ -1918,6 +1927,7 @@ sync_pipe_input_cb(GIOChannel *pipe_io, capture_session *cap_session)
         } else {
             extcap_request_stop(cap_session);
         }
+        free(buffer);
         return false;
     }

@@ -1944,6 +1954,7 @@ sync_pipe_input_cb(GIOChannel *pipe_io, capture_session *cap_session)
                "standard output", as the capture file. */
             sync_pipe_stop(cap_session);
             cap_session->closed(cap_session, NULL);
+            free(buffer);
             return false;
         }
         break;
@@ -2019,6 +2030,7 @@ sync_pipe_input_cb(GIOChannel *pipe_io, capture_session *cap_session)
         break;
     }

+    free(buffer);
     return true;
 }

diff --git a/sync_pipe.h b/sync_pipe.h
index e995a4a838..aca6338769 100644
--- a/sync_pipe.h
+++ b/sync_pipe.h
@@ -30,7 +30,7 @@
  * 4096 is a typical PIPE_BUF size for atomic writes, but we should have
  * only one writer and one reader so that shouldn't be an issue.
  */
-#define SP_MAX_MSG_LEN  24572
+#define SP_MAX_MSG_LEN  (512 * 1000)

 /*
  * Indications sent out on the sync pipe (from child to parent).
--
2.40.1

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe