You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2013/10/23 21:43:11 UTC
[Bug 55696] New: mod_jk crash in jk_map_get_int()
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
Bug ID: 55696
Summary: mod_jk crash in jk_map_get_int()
Product: Tomcat Connectors
Version: 1.2.37
Hardware: Macintosh
Status: NEW
Severity: normal
Priority: P2
Component: mod_jk
Assignee: dev@tomcat.apache.org
Reporter: sandy@clickshare.com
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);
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #14 from Rainer Jung <ra...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
Peter Aarestad <aa...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |aarestad@gmail.com
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #8 from Susan Burgee <su...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #6 from sandy@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #11 from Konstantin Kolinko <kn...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
Rainer Jung <ra...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
sandy@clickshare.com changed:
What |Removed |Added
----------------------------------------------------------------------------
OS|All |Mac OS X 10.9
--- Comment #3 from sandy@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #13 from Rainer Jung <ra...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55696
--- Comment #7 from sandy@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55696] mod_jk crash in jk_map_get_int()
Posted by bu...@apache.org.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org