Hi Serguei,
On 19/03/2013 2:47 AM, serguei.spit...@oracle.com wrote:
I already reviewed this, and it is a good fix.
Please, see my email attached.
You are not an OpenJDK Reviewer [1] . Staffan needs an official Review
from a Reviewer before it can be pushed. That doesn't mean your review
is not wanted or appreciated - it mostly certain is - but an official
Review is still required.
Cheers,
David
[1] http://openjdk.java.net/bylaws#reviewer
Thanks,
Serguei
On 3/18/13 7:13 AM, Staffan Larsen wrote:
Can I get an official Review of this change?
Thanks,
/Staffan
On 7 mar 2013, at 12:54, Staffan Larsen <staffan.lar...@oracle.com
<mailto: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/
<http://cr.openjdk.java.net/%7Esla/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
<mailto: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
<mailto: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
