bug#11374: emacsclient: avoid invalid strcpy upon partial send and buffer overrun upon failed send.

bug#11374: emacsclient: avoid invalid strcpy upon partial send and buffer overrun upon failed send.

[Jim Meyering]

This patch is untested.  I didn't even try to compile it.

From dde8915c64f020e72a141702257db91fafca5523 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sat, 28 Apr 2012 22:43:55 +0200
2012-04-28  Jim Meyering  <meyering@redhat.com>
Subject: [PATCH 2/5] emacsclient: avoid invalid strcpy upon partial send and
 buffer overrun

...upon failed send.

* lib-src/emacsclient.c (send_to_emacs): Simplify and fix two bugs:
- before, we could call strcpy with overlapping buffers upon partial
send, but strcpy cannot handle overlapping buffers.  Use memcopy.
- before, we would call strcpy(send_buffer, &send_buffer[-1]) upon
failed "send", resulting in an invalid read and a buffer overrun
when that first byte is not 0.  Diagnose the failure.
Also, call strlen just once, rather than for each iteration.
---
 lib-src/emacsclient.c |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index 48b4384..addc089 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -783,33 +783,33 @@ sock_err_message (const char *function_name)
 static void
 send_to_emacs (HSOCKET s, const char *data)
 {
-  while (data)
+  if (!data)
+    return;
+
+  size_t dlen = strlen (data);
+  while (*data)
     {
-      size_t dlen = strlen (data);
-      if (dlen + sblen >= SEND_BUFFER_SIZE)
-	{
-	  int part = SEND_BUFFER_SIZE - sblen;
-	  strncpy (&send_buffer[sblen], data, part);
-	  data += part;
-	  sblen = SEND_BUFFER_SIZE;
-	}
-      else if (dlen)
-	{
-	  strcpy (&send_buffer[sblen], data);
-	  data = NULL;
-	  sblen += dlen;
-	}
-      else
-	break;
+      size_t part = min (dlen, SEND_BUFFER_SIZE - sblen);
+      memcpy (&send_buffer[sblen], data, part);
+      data += part;
+      sblen += part;

       if (sblen == SEND_BUFFER_SIZE
 	  || (sblen > 0 && send_buffer[sblen-1] == '\n'))
 	{
 	  int sent = send (s, send_buffer, sblen, 0);
+	  if (sent < 0)
+	    {
+	      message (TRUE, "%s: failed to send %d bytes to socket: %s\n",
+		       progname, sblen, strerror (errno));
+	      fail ();
+	    }
 	  if (sent != sblen)
-	    strcpy (send_buffer, &send_buffer[sent]);
+	    memcopy (send_buffer, &send_buffer[sent], sblen - sent);
 	  sblen -= sent;
 	}
+
+      dlen -= part;
     }
 }

--
1.7.10.382.g62bc8

bug#11374: corrected patch: now, actually compiles

[Jim Meyering]

The first version lacked a definition of min and misspelled memmove.


From 3b96ff1a4f8825d3942c56c79069ca5ffea7d4f6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sat, 28 Apr 2012 22:43:55 +0200
Subject: [PATCH 2/5] emacsclient: avoid invalid strcpy upon partial send and
 buffer overrun

...upon failed send.

* lib-src/emacsclient.c (min): Define.
(send_to_emacs): Simplify and fix two bugs:
- before, we could call strcpy with overlapping buffers upon partial
send, but strcpy cannot handle overlapping buffers.  Use memmove.
- before, we would call strcpy(send_buffer, &send_buffer[-1]) upon
failed "send", resulting in an invalid read and a buffer overrun
when that first byte is not 0.  Diagnose the failure.
Also, call strlen just once, rather than for each iteration.
---
 lib-src/emacsclient.c |   38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index 48b4384..0ca13fa 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -119,6 +119,8 @@ char *(getcwd) (char *, size_t);
 # define IF_LINT(Code) /* empty */
 #endif

+#define min(x, y) (((x) < (y)) ? (x) : (y))
+
 \f
 /* Name used to invoke this program.  */
 const char *progname;
@@ -783,33 +785,33 @@ sock_err_message (const char *function_name)
 static void
 send_to_emacs (HSOCKET s, const char *data)
 {
-  while (data)
+  if (!data)
+    return;
+
+  size_t dlen = strlen (data);
+  while (*data)
     {
-      size_t dlen = strlen (data);
-      if (dlen + sblen >= SEND_BUFFER_SIZE)
-	{
-	  int part = SEND_BUFFER_SIZE - sblen;
-	  strncpy (&send_buffer[sblen], data, part);
-	  data += part;
-	  sblen = SEND_BUFFER_SIZE;
-	}
-      else if (dlen)
-	{
-	  strcpy (&send_buffer[sblen], data);
-	  data = NULL;
-	  sblen += dlen;
-	}
-      else
-	break;
+      size_t part = min (dlen, SEND_BUFFER_SIZE - sblen);
+      memcpy (&send_buffer[sblen], data, part);
+      data += part;
+      sblen += part;

       if (sblen == SEND_BUFFER_SIZE
 	  || (sblen > 0 && send_buffer[sblen-1] == '\n'))
 	{
 	  int sent = send (s, send_buffer, sblen, 0);
+	  if (sent < 0)
+	    {
+	      message (TRUE, "%s: failed to send %d bytes to socket: %s\n",
+		       progname, sblen, strerror (errno));
+	      fail ();
+	    }
 	  if (sent != sblen)
-	    strcpy (send_buffer, &send_buffer[sent]);
+	    memmove (send_buffer, &send_buffer[sent], sblen - sent);
 	  sblen -= sent;
 	}
+
+      dlen -= part;
     }
 }

--
1.7.10.382.g62bc8

bug#11374: corrected patch: now, actually compiles

[Chong Yidong]

Jim Meyering <jim@meyering.net> writes:

> The first version lacked a definition of min and misspelled memmove.
>
> From 3b96ff1a4f8825d3942c56c79069ca5ffea7d4f6 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering@redhat.com>
> Date: Sat, 28 Apr 2012 22:43:55 +0200
> Subject: [PATCH 2/5] emacsclient: avoid invalid strcpy upon partial send and
>  buffer overrun

Thanks, committed.