RFR(xs): (jdk10) 8173828: realpath is unsafe
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue Mar 14 15:20:36 UTC 2017
Thomas,
Looks good to me. Thank you for fixing it!
-Dmitry
On 2017-03-14 17:36, Thomas Stüfe wrote:
> Hi Dmitry,
>
> new
> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.02/webrev/
>
> Fixed both issues. Note that os::jvm_path() does not have a way to
> signal errors back to the caller. os::jvm_path has a number of issues
> and could benefit from a cleanup, but this is outside the scope of this fix.
>
> Thanks, Thomas
>
>
> On Tue, Mar 14, 2017 at 12:30 PM, Dmitry Samersoff
> <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>> wrote:
>
> Thomas,
>
> > thank you for the review. These are good points, I addressed them all:
>
> os_posix.cpp:
>
> 1110: it's better to write as outbuf == NULL
>
> os_solaris.cpp:
>
> 2037: return value of realpath is not checked. Could you fix it as
> well?
>
> -Dmitry
>
>
> On 2017-03-14 14:19, Thomas Stüfe wrote:
> > Hi Dmitry,
> >
> > thank you for the review. These are good points, I addressed them all:
> >
> > New
> > webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.01/webrev/
> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.01/webrev/>
> > Delta to
> > last: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.00-to-01/webrev/
> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.00-to-01/webrev/>
> >
> > Thanks, Thomas
> >
> > On Tue, Mar 14, 2017 at 10:00 AM, Dmitry Samersoff
> > <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>
> <mailto:dmitry.samersoff at oracle.com
> <mailto:dmitry.samersoff at oracle.com>>> wrote:
> >
> > Thomas,
> >
> > os_posix.cpp:
> >
> > 1121: It might be better to return ENAMETOOLONG
> >
> > 1131: It might be better to check for
> > filename != NULL && outbuf != NULL before any call to
> realpath
> > and return EINVAL for product and assert it for debug build.
> >
> > 1134: What is the goal of this assert? Do you see this problem?
> >
> > 1135: If realpath overwrites outbuf, strlen might have
> unpredictable
> > effect.
> >
> > So it might be better to set last byte of outbuf to 0 before
> > call to realpath and check that it is still zero after call
> >
> > -Dmitry
> >
> > On 2017-03-14 10:11, Thomas Stüfe wrote:
> > > Hi all,
> > >
> > > may I have reviews for this smallish fix.
> > >
> > > Issue: https://bugs.openjdk.java.net/browse/JDK-8173828
> <https://bugs.openjdk.java.net/browse/JDK-8173828>
> > <https://bugs.openjdk.java.net/browse/JDK-8173828
> <https://bugs.openjdk.java.net/browse/JDK-8173828>>
> > > Webrev:
> > >
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.00/
> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.00/>
> >
> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.00/
> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.00/>>
> > >
> > > In short, realpath(3) is unsafe the way it is traditionally used
> > (with a
> > > user buffer provided). It is safe if used in the new
> POSIX.1-2008
> > compliant
> > > way. To wrap this behavior, I added a new os::Posix::realpath()
> > function
> > > which takes a buffer and a buffer size (like a sane API
> would but the
> > > ancient realpath() did not) and moved safe buffer handling into
> > this API.
> > >
> > > Kind Regards, Thomas
> > >
> >
> >
> > --
> > Dmitry Samersoff
> > Oracle Java development team, Saint Petersburg, Russia
> > * I would love to change the world, but they won't give me the
> sources.
> >
> >
>
>
> --
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.
>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the hotspot-runtime-dev
mailing list