Here is a webrev for fixing this problem. I actually removed all of our own tokenization code in dll_build_name() and replaced it with calls to strtok_r (strtok_s on windows) instead. I think this should be more robust, at the cost of an extra memory allocation. I've also added the const qualifier to some of the parameters.
http://cr.openjdk.java.net/~sla/8009558/webrev.02/ All of the jdi and hprof tests passes with this change. Thanks, /Staffan On 6 mar 2013, at 20:48, serguei.spit...@oracle.com wrote: > Staffan, > > Thank you for the confirmation and taking care about this issue! > > Thanks, > Serguei > > > On 3/6/13 11:36 AM, Staffan Larsen wrote: >> Nice catch, Serguei! >> >> Unfortunately I have already pushed my change, but I filed "JDK-8009558: >> linked_md.c::dll_build_name can get stuck in an infinite loop" to track this >> new problem and I am working on a fix. >> >> Thanks, >> /Staffan >> >> >> On 5 mar 2013, at 20:26, serguei.spit...@oracle.com wrote: >> >>> Hi Staffan, >>> >>> Thank you for this discovery! >>> It looks good, but I have a couple of comments. >>> >>> It seems, there is one more problem in this code: >>> 68 /* check for NULL path */ >>> 69 if (p == pathname) { >>> 70 continue; <== Endless loop if we hit this line >>> 71 } >>> >>> Do we need to do 'pathname++' before continuing at the line #70? >>> It is going to be endless loop in cases there is a PATH_SEPARATOR >>> at the beginning of paths or two PATH_SEPARATOR's in a row. >>> These would be incorrect path lists but the code above is targeting exactly >>> such cases. >>> >>> Also, the argument name "pathname" in the original code is confusing. >>> Should we rename it to "pathnames"? >>> >>> Thanks, >>> Serguei >>> >>> >>> On 3/4/13 10:56 AM, Staffan Larsen wrote: >>>> I accidentally stepped on this bug the other day. There is a problem in >>>> linker_md.c : dll_build_name() where an internal pointer can be moved to >>>> point outside the string. The code looks like: >>>> >>>> 57 static void dll_build_name(char* buffer, size_t buflen, >>>> 58 const char* pname, const char* fname) { >>>> 59 // Based on os_solaris.cpp >>>> 60 >>>> 61 char *path_sep = PATH_SEPARATOR; >>>> 62 char *pathname = (char *)pname; >>>> 63 while (strlen(pathname) > 0) { >>>> 64 char *p = strchr(pathname, *path_sep); >>>> 65 if (p == NULL) { >>>> 66 p = pathname + strlen(pathname); >>>> 67 } >>>> 68 /* check for NULL path */ >>>> 69 if (p == pathname) { >>>> 70 continue; >>>> 71 } >>>> 72 (void)snprintf(buffer, buflen, "%.*s/lib%s." LIB_SUFFIX, (p - >>>> pathname), >>>> 73 pathname, fname); >>>> 74 >>>> 75 if (access(buffer, F_OK) == 0) { >>>> 76 break; >>>> 77 } >>>> 78 pathname = p + 1; >>>> 79 *buffer = '\0'; >>>> 80 } >>>> >>>> If the supplied pname is a buffer with a simple path without any path >>>> separators in it, p will be set to the terminating nul on line 66. Then on >>>> line 78 it will be moved outside the buffer. Fixing this also necessitates >>>> fixes to the callers to check for an empty return string (in buffer). >>>> >>>> The same code show up in both the solaris code and the windows code as >>>> well as hprof. >>>> >>>> bug: http://bugs.sun.com/view_bug.do?bug_id=8009397 >>>> webrev: http://cr.openjdk.java.net/~sla/8009397/webrev.00/ >>>> >>>> Thanks, >>>> /Staffan >>> >> >
