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);
What is the behavior when source and destination overlap?
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?
The behavior when they overlap: The process aborts with a log message: detected source and destination buffer overlap
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; }
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.
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 :-)
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()
(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.
(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.
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)
(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.
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.
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).
Fixed in 1583726. Will be part of version 1.2.40.