[OTR-dev] Crash when receiving message after canceling encrypted chat (with gdb backtrace)

Wed Feb 9 17:53:26 EST 2005

On Wed, Feb 09, 2005 at 04:15:21PM -0600, Evan Schoenberg wrote:
> Here's a normal output with my logging.  (The second set of information 
> in lines 2 through 4 is the pointer as output by %x).
> 	otrl_proto_create_data: starting with context->lastmessage: 
> 	"[resent] <HTML>Message sent while secure</HTML>" msg: "<HTML>Another 
> message sent while secure</HTML>"
> 	otrl_proto_create_data: will do strcpy("", "[resent] "), which is 
> strcpy(70ed248, 68596c4)
> 	otrl_proto_create_data: will do strcat("[resent] ", "<HTML>Another 
> message sent while secure</HTML>"), which is strcat(70ed248, 634ce90)
> 	otrl_proto_create_data: SUCCESS: generated [resent] <HTML>Another 
> message sent while secure</HTML> (70ed248)
> 
> Here's the crash:
> 	otrl_proto_create_data: starting with context->lastmessage: 
> 	"[resent] <HTML>Another message sent while secure</HTML>" msg: "<HTML>Now, 
> a message sent after being notified the other side is no longer using 
> encryption</HTML>"
> 	otrl_proto_create_data: will do strcpy("", "[resent] "), which is 
> strcpy(70f3e88, 68596c4)
> 	otrl_proto_create_data: will do strcat("[resent] ", "<HTML>Now, a 
> message sent after being notified the other side is no longer using 
> encryption</HTML>"), which is strcat(70f3e88, 70f03e0)
> 	otrl_proto_create_data: SUCCESS: generated [resent] <HTML>Now, a 
> message sent after being notified the other side is no longer using 
> encryption</HTML> (70f3e88)
> 	otrl_proto_create_data: starting with context->lastmessage: 
> 	"[resent] <HTML>Now, a message sent after being notified the other side is 
> no longer using encryption</HTML>", "[resent] <HTML>Now, a message sent 
> after being notified the other side is no longer using 
> encryption</HTML>"
> 	otrl_proto_create_data: will do strcpy("", "[resent] "), which is 
> strcpy(70f3e88, 68596c4)
> 	otrl_proto_create_data: will do strcat("[resent] ", msg: "[resent] 
> 	"), which is strcat(70f3e88, 70f3e88)
> 	*** malloc[19494]: error for object 0x70f1db0: Double free
> 	<CRASH>
> 
> Two interesting things I notice here... First, that method is getting 
> called twice; presumably the second time is after encryption is 
> re-established.  Second, the second call attempts to do strcat(x, x), 
> which crashes.

I was writing to say how impossible this trace is, when this old
aphorism came to mind:  "Those saying something is impossible
should never interrupt those who are doing it."

Please apply this patch and tell me if the problem goes away.

Thanks,

   - Ian

diff -u -r1.10 proto.c

--- proto.c     7 Feb 2005 20:34:50 -0000       1.10
+++ proto.c     9 Feb 2005 22:46:31 -0000
@@ -735,6 +735,14 @@
     char *msgbuf = NULL;
     enum gcry_mpi_format format = GCRYMPI_FMT_USG;

+    /* We need to copy the incoming msg, since it might be an alias for
+     * context->lastmessage, which we'll be freeing soon. */
+    char *msgdup = gcry_malloc_secure(justmsglen + 1);
+    if (msgdup == NULL) {
+       return gcry_error(GPG_ERR_ENOMEM);
+    }
+    strcpy(msgdup, msg);
+
     *encmessagep = NULL;

     /* Header, send keyid, recv keyid, counter, msg len, msg
@@ -746,10 +754,11 @@
     msgbuf = gcry_malloc_secure(msglen);
     if (buf == NULL || msgbuf == NULL) {
        free(buf);
-       free(msgbuf);
+       gcry_free(msgbuf);
+       gcry_free(msgdup);
        return gcry_error(GPG_ERR_ENOMEM);
     }
-    memmove(msgbuf, msg, justmsglen);
+    memmove(msgbuf, msgdup, justmsglen);
     msgbuf[justmsglen] = '\0';
     otrl_tlv_serialize(msgbuf + justmsglen + 1, tlvs);
     bufp = buf;
@@ -824,7 +833,7 @@
     if (msglen > 0) {
        const char *prefix = "[resent] ";
        size_t prefixlen = strlen(prefix);
-       if (!strncmp(prefix, msg, prefixlen)) {
+       if (!strncmp(prefix, msgdup, prefixlen)) {
            /* The prefix is already there.  Don't add it again. */
            prefix = "";
            prefixlen = 0;
@@ -832,13 +841,15 @@
        context->lastmessage = gcry_malloc_secure(prefixlen + justmsglen + 1);
        if (context->lastmessage) {
            strcpy(context->lastmessage, prefix);
-           strcat(context->lastmessage, msg);
+           strcat(context->lastmessage, msgdup);
        }
     }
+    gcry_free(msgdup);
     return gcry_error(GPG_ERR_NO_ERROR);
 err:
     free(buf);
     gcry_free(msgbuf);
+    gcry_free(msgdup);
     *encmessagep = NULL;
     return err;
 }


