[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #13 from Rainer Jung rainer.j...@kippdata.de ---
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).
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 --- Comment #14 from Rainer Jung rainer.j...@kippdata.de --- Fixed in 1583726. Will be part of version 1.2.40. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 Rainer Jung rainer.j...@kippdata.de changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 --- Comment #11 from Konstantin Kolinko knst.koli...@gmail.com --- (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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 --- Comment #12 from Christopher Schultz ch...@christopherschultz.net --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #2 from Christopher Schultz ch...@christopherschultz.net ---
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?
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 sa...@clickshare.com changed: What|Removed |Added OS|All |Mac OS X 10.9 --- Comment #3 from sa...@clickshare.com --- The behavior when they overlap: The process aborts with a log message: detected source and destination buffer overlap -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 Christopher Schultz ch...@christopherschultz.net changed: What|Removed |Added OS||All -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #4 from Christopher Schultz ch...@christopherschultz.net ---
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;
}
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 --- Comment #5 from Christopher Schultz ch...@christopherschultz.net --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 --- Comment #6 from sa...@clickshare.com --- 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 :-) -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #7 from sa...@clickshare.com ---
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()
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 --- Comment #8 from Susan Burgee susa...@yahoo.com --- (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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 --- Comment #9 from Christopher Schultz ch...@christopherschultz.net --- (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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #10 from Christopher Schultz ch...@christopherschultz.net ---
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)
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 Christopher Schultz ch...@christopherschultz.net changed: What|Removed |Added OS||All --- Comment #1 from Christopher Schultz ch...@christopherschultz.net --- What is the behavior when source and destination overlap? -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696 Peter Aarestad aares...@gmail.com changed: What|Removed |Added CC||aares...@gmail.com -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
