Can I get an official Review of this change? Thanks, /Staffan
On 7 mar 2013, at 12:54, Staffan Larsen <staffan.lar...@oracle.com> wrote: > 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 >>>> >>> >> >
