RFR(xs): (jdk10) 8173828: realpath is unsafe

[Thomas Stüfe]

Thanks for the review!

On Tue, Mar 14, 2017 at 4:20 PM, Dmitry Samersoff <
dmitry.samersoff at oracle.com> wrote:

> 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.
>