Put special pipe-handling code into libwsutil.

Ask, in a comment, why we're doing PeekNamedPipe() when we're trying
to read everyting in the pipe, up to the EOF, into a string.

On UN*X, do the same "read up to an EOF and then NUL-terminate the
result" stuff that we did on Windows; nothing guarantees that, on all
UN*Xes, in all circumstances, until the end of time, world without end,
amen, we can do one read and get the entire string.

Change-Id: I578802b23fec1051139eaefd9a09fe2a6de06a11
Reviewed-on: https://code.wireshark.org/review/24959
Petri-Dish: Guy Harris <guy@alum.mit.edu>
Tested-by: Petri Dish Buildbot
Reviewed-by: Guy Harris <guy@alum.mit.edu>

8 files changed, 237 insertions, 120 deletions

diff --git a/capchild/capture_sync.c b/capchild/capture_sync.c
index e8d6fcf10b..43fc2ac173 100644
--- a/capchild/capture_sync.c
+++ b/capchild/capture_sync.c
@@ -100,7 +100,7 @@
 #include <process.h>    /* For spawning child process */
 #endif
 
-
+#include <wsutil/ws_pipe.h>
 
 #ifdef _WIN32
 static void create_dummy_signal_pipe();
@@ -1524,37 +1524,12 @@ pipe_read_bytes(int pipe_fd, char *bytes, int required, char **msg)
     return offset;
 }
 
-static gboolean pipe_data_available(int pipe_fd) {
-#ifdef _WIN32 /* PeekNamedPipe */
-    HANDLE hPipe = (HANDLE) _get_osfhandle(pipe_fd);
-    DWORD bytes_avail;
-
-    if (hPipe == INVALID_HANDLE_VALUE)
-        return FALSE;
-
-    if (! PeekNamedPipe(hPipe, NULL, 0, NULL, &bytes_avail, NULL))
-        return FALSE;
-
-    if (bytes_avail > 0)
-        return TRUE;
-    return FALSE;
-#else /* select */
-    fd_set rfds;
-    struct timeval timeout;
-
-    FD_ZERO(&rfds);
-    FD_SET(pipe_fd, &rfds);
-    timeout.tv_sec = 0;
-    timeout.tv_usec = 0;
-
-    if (select(pipe_fd+1, &rfds, NULL, NULL, &timeout) > 0)
-        return TRUE;
-
-    return FALSE;
-#endif
-}
-
-/* Read a line from a pipe, similar to fgets */
+/*
+ * Read a line from a pipe; similar to fgets, but doesn't block.
+ *
+ * XXX - just stops reading if there's nothing to be read right now;
+ * that could conceivably mean that you don't get a complete line.
+ */
 int
 sync_pipe_gets_nonblock(int pipe_fd, char *bytes, int max) {
     ssize_t newly;
@@ -1562,7 +1537,7 @@ sync_pipe_gets_nonblock(int pipe_fd, char *bytes, int max) {
 
     while(offset < max - 1) {
         offset++;
-        if (! pipe_data_available(pipe_fd))
+        if (! ws_pipe_data_available(pipe_fd))
             break;
         newly = ws_read(pipe_fd, &bytes[offset], 1);
         if (newly == 0) {
diff --git a/extcap.c b/extcap.c
index d0d527ffb1..03aada8a9f 100644
--- a/extcap.c
+++ b/extcap.c
@@ -42,6 +42,7 @@
 #include <wsutil/glib-compat.h>
 #include <wsutil/file_util.h>
 #include <wsutil/filesystem.h>
+#include <wsutil/ws_pipe.h>
 #include <wsutil/tempfile.h>
 
 #include "capture_opts.h"
@@ -944,46 +945,6 @@ extcap_has_toolbar(const char *ifname)
     return FALSE;
 }
 
-/* taken from capchild/capture_sync.c */
-static gboolean pipe_data_available(int pipe_fd)
-{
-#ifdef _WIN32 /* PeekNamedPipe */
-    HANDLE hPipe = (HANDLE) _get_osfhandle(pipe_fd);
-    DWORD bytes_avail;
-
-    if (hPipe == INVALID_HANDLE_VALUE)
-    {
-        return FALSE;
-    }
-
-    if (! PeekNamedPipe(hPipe, NULL, 0, NULL, &bytes_avail, NULL))
-    {
-        return FALSE;
-    }
-
-    if (bytes_avail > 0)
-    {
-        return TRUE;
-    }
-    return FALSE;
-#else /* select */
-    fd_set rfds;
-    struct timeval timeout;
-
-    FD_ZERO(&rfds);
-    FD_SET(pipe_fd, &rfds);
-    timeout.tv_sec = 0;
-    timeout.tv_usec = 0;
-
-    if (select(pipe_fd + 1, &rfds, NULL, NULL, &timeout) > 0)
-    {
-        return TRUE;
-    }
-
-    return FALSE;
-#endif
-}
-
 void extcap_if_cleanup(capture_options *capture_opts, gchar **errormsg)
 {
     interface_options *interface_opts;
@@ -1064,23 +1025,10 @@ void extcap_if_cleanup(capture_options *capture_opts, gchar **errormsg)
         userdata = (extcap_userdata *) interface_opts->extcap_userdata;
         if (userdata)
         {
-            if (userdata->extcap_stderr_rd > 0 && pipe_data_available(userdata->extcap_stderr_rd))
+            if (userdata->extcap_stderr_rd > 0 && ws_pipe_data_available(userdata->extcap_stderr_rd))
             {
-                buffer = (gchar *)g_malloc0(sizeof(gchar) * STDERR_BUFFER_SIZE + 1);
-#ifdef _WIN32
-                win32_readfrompipe((HANDLE)_get_osfhandle(userdata->extcap_stderr_rd), STDERR_BUFFER_SIZE, buffer);
-#else
-                ssize_t buffer_len;
-                buffer_len = read(userdata->extcap_stderr_rd, buffer, sizeof(gchar) * STDERR_BUFFER_SIZE);
-                if (buffer_len <= 0)
-                {
-                    buffer[0] = '\0';
-                }
-                else
-                {
-                    buffer[buffer_len] = '\0';
-                }
-#endif
+                buffer = (gchar *)g_malloc0(STDERR_BUFFER_SIZE + 1);
+                ws_read_string_from_pipe(ws_get_pipe_handle(userdata->extcap_stderr_rd), buffer, STDERR_BUFFER_SIZE + 1);
                 if (strlen(buffer) > 0)
                 {
                     userdata->extcap_stderr = g_strdup_printf("%s", buffer);
diff --git a/extcap_spawn.c b/extcap_spawn.c
index a5d9f4df53..42434df819 100644
--- a/extcap_spawn.c
+++ b/extcap_spawn.c
@@ -18,6 +18,7 @@
 
 #include <wsutil/file_util.h>
 #include <wsutil/filesystem.h>
+#include <wsutil/ws_pipe.h>
 #ifdef _WIN32
 #include <wsutil/win32-utils.h>
 #endif
@@ -27,35 +28,6 @@
 #include "extcap.h"
 #include "extcap_spawn.h"
 
-#ifdef _WIN32
-
-void win32_readfrompipe(HANDLE read_pipe, gint32 max_buffer, gchar *buffer)
-{
-    gboolean bSuccess = FALSE;
-    gint32 bytes_written = 0;
-    gint32 max_bytes = 0;
-
-    DWORD dwRead;
-    DWORD bytes_avail = 0;
-
-    for (;;)
-    {
-        if (!PeekNamedPipe(read_pipe, NULL, 0, NULL, &bytes_avail, NULL)) break;
-        if (bytes_avail <= 0) break;
-
-        max_bytes = max_buffer - bytes_written - 1;
-
-        bSuccess = ReadFile(read_pipe, &buffer[bytes_written], max_bytes, &dwRead, NULL);
-        if (!bSuccess || dwRead == 0) break;
-
-        bytes_written += dwRead;
-        if ((bytes_written + 1) >= max_buffer) break;
-    }
-
-    buffer[bytes_written] = '\0';
-}
-#endif
-
 gboolean extcap_spawn_sync(gchar *dirname, gchar *command, gint argc, gchar **args, gchar **command_output)
 {
     gboolean status = FALSE;
@@ -148,7 +120,7 @@ gboolean extcap_spawn_sync(gchar *dirname, gchar *command, gint argc, gchar **ar
     if (CreateProcess(NULL, wcommandline, NULL, NULL, TRUE, CREATE_NEW_CONSOLE, NULL, NULL, &info, &processInfo))
     {
         WaitForSingleObject(processInfo.hProcess, INFINITE);
-        win32_readfrompipe(child_stdout_rd, BUFFER_SIZE, buffer);
+        ws_read_string_from_pipe(child_stdout_rd, buffer, BUFFER_SIZE);
         local_output = g_strdup_printf("%s", buffer);
 
         CloseHandle(child_stdout_rd);
diff --git a/extcap_spawn.h b/extcap_spawn.h
index 7296cd9bb6..fafce74268 100644
--- a/extcap_spawn.h
+++ b/extcap_spawn.h
@@ -36,7 +36,6 @@ GPid extcap_spawn_async ( extcap_userdata * userdata, GPtrArray * args );
 
 #ifdef _WIN32
 gboolean extcap_wait_for_pipe(HANDLE * pipe_handles, int num_pipe_handles, HANDLE pid);
-void win32_readfrompipe(HANDLE read_pipe, gint32 max_buffer, gchar * buffer);
 #endif
 
 #endif
diff --git a/wsutil/CMakeLists.txt b/wsutil/CMakeLists.txt
index dbd8a95b99..5f27e74780 100644
--- a/wsutil/CMakeLists.txt
+++ b/wsutil/CMakeLists.txt
@@ -78,6 +78,7 @@ set(WSUTIL_PUBLIC_HEADERS
 	ws_cpuid.h
 	ws_mempbrk.h
 	ws_mempbrk_int.h
+	ws_pipe.h
 	ws_printf.h
 	wsjsmn.h
 	xtea.h
@@ -127,6 +128,7 @@ set(WSUTIL_COMMON_FILES
 	type_util.c
 	unicode-utils.c
 	ws_mempbrk.c
+	ws_pipe.c
 	wsgcrypt.c
 	wsjsmn.c
 	xtea.c
diff --git a/wsutil/Makefile.am b/wsutil/Makefile.am
index 6cb212bbf9..72fb638866 100644
--- a/wsutil/Makefile.am
+++ b/wsutil/Makefile.am
@@ -86,6 +86,7 @@ WSUTIL_PUBLIC_INCLUDES = \
 	ws_cpuid.h		\
 	ws_mempbrk.h		\
 	ws_mempbrk_int.h	\
+	ws_pipe.h		\
 	ws_printf.h		\
 	wsjsmn.h		\
 	wsgcrypt.h		\
@@ -164,6 +165,7 @@ libwsutil_la_SOURCES = \
 	type_util.c		\
 	unicode-utils.c		\
 	ws_mempbrk.c		\
+	ws_pipe.c		\
 	wsgcrypt.c		\
 	wsjsmn.c		\
 	xtea.c
diff --git a/wsutil/ws_pipe.c b/wsutil/ws_pipe.c
new file mode 100644
index 0000000000..7f3f16937b
--- /dev/null
+++ b/wsutil/ws_pipe.c
@@ -0,0 +1,187 @@
+/* ws_pipe.c
+ *
+ * Routines for handling pipes.
+ *
+ * Wireshark - Network traffic analyzer
+ * By Gerald Combs <gerald@wireshark.org>
+ * Copyright 1998 Gerald Combs
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#ifdef _WIN32
+#include <windows.h>
+#include <io.h>
+#else
+#include <unistd.h>
+#endif
+
+#include <glib.h>
+
+#include "wsutil/ws_pipe.h"
+
+gboolean
+ws_pipe_data_available(int pipe_fd)
+{
+#ifdef _WIN32 /* PeekNamedPipe */
+    HANDLE hPipe = (HANDLE) _get_osfhandle(pipe_fd);
+    DWORD bytes_avail;
+
+    if (hPipe == INVALID_HANDLE_VALUE)
+    {
+        return FALSE;
+    }
+
+    if (! PeekNamedPipe(hPipe, NULL, 0, NULL, &bytes_avail, NULL))
+    {
+        return FALSE;
+    }
+
+    if (bytes_avail > 0)
+    {
+        return TRUE;
+    }
+    return FALSE;
+#else /* select */
+    fd_set rfds;
+    struct timeval timeout;
+
+    FD_ZERO(&rfds);
+    FD_SET(pipe_fd, &rfds);
+    timeout.tv_sec = 0;
+    timeout.tv_usec = 0;
+
+    if (select(pipe_fd + 1, &rfds, NULL, NULL, &timeout) > 0)
+    {
+        return TRUE;
+    }
+
+    return FALSE;
+#endif
+}
+
+gboolean
+ws_read_string_from_pipe(ws_pipe_handle read_pipe, gchar *buffer,
+                         size_t buffer_size)
+{
+    size_t total_bytes_read;
+    size_t buffer_bytes_remaining;
+#ifdef _WIN32
+    DWORD bytes_to_read;
+    DWORD bytes_read;
+    DWORD bytes_avail;
+#else
+    size_t bytes_to_read;
+    ssize_t bytes_read;
+#endif
+
+    if (buffer_size == 0)
+    {
+        /* XXX - provide an error string */
+        return FALSE;
+    }
+    if (buffer_size == 1)
+    {
+        /* No room for an actual string */
+        buffer[0] = '\0';
+        return TRUE;
+    }
+
+    /*
+     * Number of bytes of string data we can actually read, leaving room
+     * for the terminating NUL.
+     */
+    buffer_size--;
+
+    total_bytes_read = 0;
+    for (;;)
+    {
+        buffer_bytes_remaining = buffer_size - total_bytes_read;
+        if (buffer_bytes_remaining == 0)
+        {
+            /* The string won't fit in the buffer. */
+            /* XXX - provide an error string */
+            return FALSE;
+        }
+
+#ifdef _WIN32
+        /*
+         * XXX - is there some reason why we do this before reading?
+         *
+         * If we're not trying to do UN*X-style non-blocking I/O,
+         * where we don't block if there isn't data available to
+         * read right now, I'm not sure why we do this.
+         *
+         * If we *are* trying to do UN*X-style non-blocking I/O,
+         * 1) we're presumably in an event loop waiting for,
+         * among other things, input to be available on the
+         * pipe, in which case we should be doing "overlapped"
+         * I/O and 2) we need to accumulate data until we have
+         * a complete string, rather than just saying "OK, here's
+         * the string".)
+         */
+        if (!PeekNamedPipe(read_pipe, NULL, 0, NULL, &bytes_avail, NULL))
+        {
+            break;
+        }
+        if (bytes_avail <= 0)
+        {
+            break;
+        }
+
+        /*
+         * Truncate this to whatever fits in a DWORD.
+         */
+        if (buffer_bytes_remaining > 0x7fffffff)
+        {
+            bytes_to_read = 0x7fffffff;
+        }
+        else
+        {
+            bytes_to_read = (DWORD)buffer_bytes_remaining;
+        }
+        if (!ReadFile(read_pipe, &buffer[total_bytes_read], bytes_to_read,
+            &bytes_read, NULL))
+        {
+            /* XXX - provide an error string */
+            return FALSE;
+        }
+#else
+        bytes_to_read = buffer_bytes_remaining;
+        bytes_read = read(read_pipe, buffer, bytes_to_read);
+        if (bytes_read == -1)
+        {
+            /* XXX - provide an error string */
+            return FALSE;
+        }
+#endif
+        if (bytes_read == 0)
+        {
+            break;
+        }
+
+        total_bytes_read += bytes_read;
+    }
+
+    buffer[total_bytes_read] = '\0';
+    return TRUE;
+}
+
+/*
+ * Editor modelines  -  http://www.wireshark.org/tools/modelines.html
+ *
+ * Local variables:
+ * c-basic-offset: 4
+ * tab-width: 8
+ * indent-tabs-mode: nil
+ * End:
+ *
+ * vi: set shiftwidth=4 tabstop=8 expandtab:
+ * :indentSize=4:tabSize=8:noTabs=true:
+ */
diff --git a/wsutil/ws_pipe.h b/wsutil/ws_pipe.h
new file mode 100644
index 0000000000..b52478a8e0
--- /dev/null
+++ b/wsutil/ws_pipe.h
@@ -0,0 +1,32 @@
+/* ws_pipe.h
+ *
+ * Routines for handling pipes.
+ *
+ * Wireshark - Network traffic analyzer
+ * By Gerald Combs <gerald@wireshark.org>
+ * Copyright 1998 Gerald Combs
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#ifndef __WS_PIPE_H__
+#define __WS_PIPE_H__
+
+#include "ws_symbol_export.h"
+
+#ifdef _WIN32
+#include <windows.h>
+#include <io.h>
+#define ws_pipe_handle			HANDLE
+#define ws_get_pipe_handle(pipe_fd)	((HANDLE)_get_osfhandle(pipe_fd))
+#else
+#define ws_pipe_handle			int
+#define ws_get_pipe_handle(pipe_fd)	(pipe_fd)
+#endif
+
+WS_DLL_PUBLIC gboolean ws_pipe_data_available(int pipe_fd);
+
+WS_DLL_PUBLIC gboolean ws_read_string_from_pipe(ws_pipe_handle read_pipe,
+    gchar *buffer, size_t buffer_size);
+
+#endif /* __WS_PIPE_H__ */