Bug 55696 - mod_jk crash in jk_map_get_int()
Summary: mod_jk crash in jk_map_get_int()
Status: RESOLVED FIXED
Alias: None
Product: Tomcat Connectors
Classification: Unclassified
Component: mod_jk (show other bugs)
Version: 1.2.37
Hardware: Macintosh All
: P2 normal with 10 votes (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-23 19:43 UTC by sandy
Modified: 2014-04-01 16:12 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description sandy 2013-10-23 19:43:11 UTC
Apple appears to have made strcpy() enforce that the src and dest buffers may not overlap. As a result jk_map_get_int() may fail on the line strcpy(but, rc); as rc may be set to but by jk_map_get_string().

As a work around, I have created a second buffer char buf2[100] and used that for def:

 sprintf(buf2, "%d", def);
 rc = jk_map_get_string(m, name, buf2);
Comment 1 Christopher Schultz 2013-10-23 21:15:32 UTC
What is the behavior when source and destination overlap?
Comment 2 Christopher Schultz 2013-10-24 12:21:06 UTC
Honestly, I'm not even sure why the code is written this way.

  char buf[];
  sprintf(buf, "%d", default_value);
  char *rc = jk_map_get_string(m, name, buf);
  size_t len = strlen(rc);
  if(len)  {
     // parse rc -> int_res
  } else {
    int_res = default_value;
  }

  return int_res;

Why bother at all with the whole default int -> string -> int garbage? Why not simply pass NULL as the default value to jk_map_get_string and check for either NULL or 0==len afterward?
Comment 3 sandy 2013-10-24 13:06:44 UTC
The behavior when they overlap:

The process aborts with a log message: detected source and destination buffer overlap
Comment 4 Christopher Schultz 2013-10-24 13:13:08 UTC
So, SIGABRT?

I'm trying to reproduce in a test-case, but this doesn't seem to work:

#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[]) {
  char *a = "Hello, world!";
  char buf1[50];

  strcpy(buf1, a);

  strcpy(buf1 + 7, buf1);

  printf("buf1 is now %s\n", buf1);
  return 0;
}
Comment 5 Christopher Schultz 2013-10-24 13:15:31 UTC
strcpy(buf1, buf1) also does not fail.

$ cc --version
Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)
Target: x86_64-apple-darwin12.5.0
Thread model: posix

Hmm... did you just upgrade to Mavericks? I haven't yet upgraded so maybe that's the problem.
Comment 6 sandy 2013-10-24 13:19:26 UTC
Yes, this problem is new with Mavericks (10.9). It was working fine on 10.8

I will try to create you a test case... Unfortunately, It has been about 25 years since I did any C memory management -- so it may take me a little while :-)
Comment 7 sandy 2013-10-24 13:29:56 UTC
This is the reductive case when m && name returns false in jk_map_get_string() 

#include <stdio.h>
#include <string.h>

int main(int argc, const char * argv[])
{
    char buf[50] = "Hello, world";
    const char *rc;
    
    rc = buf;
    
    strcpy(buf, rc);
}

Thread 1: signal SIGABRT in __strcpy_chk()
Comment 8 Susan Burgee 2013-10-24 15:30:35 UTC
(In reply to sandy from comment #0)
> Apple appears to have made strcpy() enforce that the src and dest buffers
> may not overlap. As a result jk_map_get_int() may fail on the line
> strcpy(but, rc); as rc may be set to but by jk_map_get_string().
> 
> As a work around, I have created a second buffer char buf2[100] and used
> that for def:
> 
>  sprintf(buf2, "%d", def);
>  rc = jk_map_get_string(m, name, buf2);

I applied this fix after upgrading to Mavericks and it solved the problem.
Comment 9 Christopher Schultz 2013-10-24 18:03:04 UTC
(In reply to Susan Burgee from comment #8)
> (In reply to sandy from comment #0)
> > Apple appears to have made strcpy() enforce that the src and dest buffers
> > may not overlap. As a result jk_map_get_int() may fail on the line
> > strcpy(but, rc); as rc may be set to but by jk_map_get_string().
> > 
> > As a work around, I have created a second buffer char buf2[100] and used
> > that for def:
> > 
> >  sprintf(buf2, "%d", def);
> >  rc = jk_map_get_string(m, name, buf2);
> 
> I applied this fix after upgrading to Mavericks and it solved the problem.

Exactly which fix? Just adding a separate "buf2[]"? My fix is likely to remove the use of sprintf and avoid the problem entirely.
Comment 10 Christopher Schultz 2013-10-24 21:14:15 UTC
I have verified that this patch works under both Linux and OS X Mavericks. I'll commit shortly.

Index: native/common/jk_map.c
===================================================================
--- native/common/jk_map.c	(revision 1535519)
+++ native/common/jk_map.c	(working copy)
@@ -183,33 +183,37 @@
 
 int jk_map_get_int(jk_map_t *m, const char *name, int def)
 {
-    char buf[100];
     const char *rc;
-    size_t len;
     int int_res;
-    int multit = 1;
 
-    sprintf(buf, "%d", def);
-    rc = jk_map_get_string(m, name, buf);
+    rc = jk_map_get_string(m, name, NULL);
 
-    len = strlen(rc);
-    if (len) {
-        char *lastchar = &buf[0] + len - 1;
-        strcpy(buf, rc);
-        if ('m' == *lastchar || 'M' == *lastchar) {
-            *lastchar = '\0';
-            multit = 1024 * 1024;
+    if(NULL == rc) {
+        int_res = def;
+    } else {
+        size_t len = strlen(rc);
+        int multit = 1;
+
+        if (len) {
+            char buf[100];
+            char *lastchar;
+            strncpy(buf, rc, 100);
+	    lastchar = buf + len - 1;
+            if ('m' == *lastchar || 'M' == *lastchar) {
+                *lastchar = '\0';
+                multit = 1024 * 1024;
+            }
+            else if ('k' == *lastchar || 'K' == *lastchar) {
+                *lastchar = '\0';
+                multit = 1024;
+            }
+            int_res = multit * atoi(buf);
         }
-        else if ('k' == *lastchar || 'K' == *lastchar) {
-            *lastchar = '\0';
-            multit = 1024;
-        }
-        int_res = atoi(buf);
+        else
+            int_res = def;
     }
-    else
-        int_res = def;
 
-    return int_res * multit;
+    return int_res;
 }
 
 double jk_map_get_double(jk_map_t *m, const char *name, double def)
Comment 11 Konstantin Kolinko 2013-10-25 10:55:20 UTC
(In reply to Christopher Schultz from comment #10)

Looks good.

Some style comments
1. Beware tabs. This line is formatted oddly because it has a tab character:
> +	    lastchar = buf + len - 1;

2. 
> +        int multit = 1;

Can be moved several lines below into the if(len) block.

Or definition of "char buf[100];" could be moved up just below that line. (see below).

3.
> +            strncpy(buf, rc, 100);

s/100/sizeof(buf)/ ?

4. strncpy is not a safe function. It the string is longer than 100 characters it will truncate it without setting a 0 byte at the end.

Thus in other places it is actually strncpy(..., size-1), and it needs some code to explicitly set the size'th byte of the buffer to 0.

Moreover, you are not recalculating "len" after copying thus if len > 100, the lastchar pointer will point to some place outside the buffer:

> +	    lastchar = buf + len - 1;

Thus maybe s/ if(len) / if (len && len < sizeof(buf)-1) / to resort to the defaults in this case.
Comment 12 Christopher Schultz 2013-10-25 15:54:18 UTC
Konstantin,

Yeah, sorry about the tabs. I used vi in stupid-mode. I'll get close cleaned-up before a commit.

As for the stncpy, I was originally thinking that an int couldn't be longer than a few characters, but on further reflection, it doesn't matter: instead, its the user input that must be fewer than 100 characters if this isn't going to fail.

I decided to use strncpy because the existing code used strcpy which was IMO even worse. I was thinking I might make a bigger change to use strtol() and actually look at the value of 'endptr' after the call. I didn't want a patch that made too many changes at once.

Before my patch, the strcpy was happening *after* the use of len. I'll clean that up, too.

Using strtol (instead of atoi) will do a better job of detecting problems with the actual "value" coming from the user. Right now, if you say worker.port=abc, then atoi will return an undefined value (probably 0) for that configuration option. I'll fix the other stuff and then look at using strtol.
Comment 13 Rainer Jung 2014-04-01 15:55:36 UTC
A more minimalistic patch would be (untested yet):

Index: common/jk_map.c
===================================================================
--- common/jk_map.c     (revision 1583423)
+++ common/jk_map.c     (working copy)
@@ -206,7 +206,6 @@
     const char *rc;
     size_t len;
     int int_res;
-    int multit = 1;

     sprintf(buf, "%d", def);
     rc = jk_map_get_string(m, name, buf);
@@ -213,22 +212,21 @@

     len = strlen(rc);
     if (len) {
-        char *lastchar = &buf[0] + len - 1;
-        strcpy(buf, rc);
+        const char *lastchar = &rc[0] + len - 1;
+        int multit = 1;
         if ('m' == *lastchar || 'M' == *lastchar) {
-            *lastchar = '\0';
             multit = 1024 * 1024;
         }
         else if ('k' == *lastchar || 'K' == *lastchar) {
-            *lastchar = '\0';
             multit = 1024;
         }
-        int_res = atoi(buf);
+        /* Safe because atoi() will stop at any non-numeric lastchar */
+        int_res = atoi(rc) * multit;
     }
     else
         int_res = def;

-    return int_res * multit;
+    return int_res;
 }

 double jk_map_get_double(jk_map_t *m, const char *name, double def)


The only reason for using a copy of rc seems to be that we terminate the string after the number in case there was a scaling character ("k" or "M" etc.). But atoi should stop converting to a number when detecting such a character anyhow, so we don't nee to overwrite it with a zero byte and thus can operate on the original rc. No string copy, no overlap.

Moving the use of multit completely to the non-default value block because it is a bit misleading to multiply in the default case (factor was "1" there).
Comment 14 Rainer Jung 2014-04-01 16:11:24 UTC
Fixed in 1583726.
Will be part of version 1.2.40.