RFR(xs): (jdk10) 8173828: realpath is unsafe
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Mar 16 07:24:52 UTC 2017
Hi Chris, David,
Can one of you sponsor this change? Thanks!
..Thomas
On Thu, Mar 16, 2017 at 6:36 AM, Dmitry Samersoff <
dmitry.samersoff at oracle.com> wrote:
> Thomas,
>
> Looks good to me.
>
> -Dmitry
>
> On 2017-03-15 13:15, Thomas Stüfe wrote:
> > Hi David,
> >
> > here the latest requested changes:
> >
> > http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
> realpath-is-unsafe/jdk10-webrev.04/webrev/
> >
> > Kind Regards, Thomas
> >
> >
> > On Wed, Mar 15, 2017 at 11:02 AM, David Holmes <david.holmes at oracle.com>
> > wrote:
> >
> >> Hi Thomas,
> >>
> >> On 15/03/2017 6:41 PM, Thomas Stüfe wrote:
> >>
> >>> Hi all,
> >>>
> >>> thanks for the reviews. I addressed the remaining issues:
> >>>
> >>> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-
> >>> is-unsafe/jdk10-webrev.03/webrev/
> >>> Delta to
> >>> last: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-
> >>> is-unsafe/jdk10-webrev.02-to-03.diff
> >>>
> >>
> >> There is a second use of overwriter in the guarantee.
> >>
> >> In:
> >>
> >> // On truncation error ( 'outbuf'
> >>
> >> can you remove the space after the ( please.
> >>
> >> Thanks for the thumbs up. Interesting that no-one commented on the
> >>> guarantee in case of an overwrite, you are all ok with it? Thought
> about
> >>> using an assert, but I do not really want the production VM to ignore
> >>> overwrites.
> >>>
> >>
> >> I think that given this should be on the uncommon path and that we
> should
> >> be passing in a big enough buffer, and that buffer overwrites are bad,
> then
> >> the guarantee is ok.
> >>
> >> Thanks,
> >> David
> >>
> >> Kind Regards, Thomas
> >>>
> >>>
> >>>
> >>>
> >>> On Wed, Mar 15, 2017 at 8:30 AM, Langer, Christoph
> >>> <christoph.langer at sap.com <mailto:christoph.langer at sap.com>> wrote:
> >>>
> >>> Hi Thomas,
> >>>
> >>> apart from David's findings I have only to add that the copyright
> >>> year in os_posix.cpp should be updated as well.
> >>>
> >>> +1 from me.
> >>>
> >>> Best regards
> >>> Christoph
> >>>
> >>> > -----Original Message-----
> >>> > From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> >>> <mailto:hotspot-runtime-dev->
> >>> > bounces at openjdk.java.net <mailto:bounces at openjdk.java.net>] On
> >>> Behalf Of David Holmes
> >>> > Sent: Mittwoch, 15. März 2017 02:18
> >>> > To: Thomas Stüfe <thomas.stuefe at gmail.com
> >>> <mailto:thomas.stuefe at gmail.com>>; Dmitry Samersoff
> >>> > <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com
> >>
> >>> > Cc: hotspot-runtime-dev at openjdk.java.net
> >>> <mailto:hotspot-runtime-dev at openjdk.java.net>
> >>>
> >>> > Subject: Re: RFR(xs): (jdk10) 8173828: realpath is unsafe
> >>> >
> >>> > Hi Thomas,
> >>> >
> >>> > Generally looks good. A few minor comments below.
> >>> >
> >>> > On 15/03/2017 12:36 AM, Thomas Stüfe wrote:
> >>> > > Hi Dmitry,
> >>> > >
> >>> > > new webrev:
> >>> > > http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
> realpath-is-
> >>> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is->
> >>> > unsafe/jdk10-webrev.02/webrev/
> >>> >
> >>> > os_solaris.cpp:
> >>> >
> >>> > + char* rp = NULL;
> >>> >
> >>> > This new local variable seems unused.
> >>> >
> >>> > ---
> >>> >
> >>> > src/os/posix/vm/os_posix.hpp
> >>> >
> >>> > The comments are a little confusing:
> >>> >
> >>> > + // On success, it will return a pointer to the input buffer.
> >>> > + // On error, it will return NULL and set errno. Content of
> >>> output
> >>> > buffer is undefined.
> >>> > + // On truncation error (output buffer too small), it will
> return
> >>> > NULL and set errno to ENAMETOOLONG.
> >>> >
> >>> > Is it an input buffer or an output buffer? I think we are
> inputting
> >>> an
> >>> > output buffer :) Perhaps:
> >>> >
> >>> > // On success, returns 'outbuf', which now contains the path.
> >>> > // On error, it will return NULL and set errno. The content of
> >>> 'outbuf'
> >>> > is undefined.
> >>> > // On truncation error ( 'outbuf' too small), it will return NULL
> >>> and
> >>> > set errno to ENAMETOOLONG.
> >>> >
> >>> > ---
> >>> >
> >>> > src/os/posix/vm/os_posix.cpp
> >>> >
> >>> > Grammatical nit:
> >>> >
> >>> > + // that it complains about the NULL we did hand down as
> user
> >>> buffer.
> >>> >
> >>> > change to
> >>> >
> >>> > + // that it complains about the NULL we handed down as the
> >>> user buffer.
> >>> >
> >>> > Nit:
> >>> >
> >>> > + // a memory overwriter.
> >>> > + guarantee(outbuf[outbuflen - 1] == '\0', "realpath
> buffer
> >>> > overwriter detected.");
> >>> >
> >>> > overwiter -> overwite
> >>> >
> >>> > ---
> >>> >
> >>> > Thanks,
> >>> > David
> >>> > -----
> >>> >
> >>> >
> >>> > > 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-
> >>> <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-
> >>> <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 oracl
> >>> e.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-
> >>> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828->
> >>> > >> realpath-is-unsafe/jdk10-webrev.00/
> >>> > >>> <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
> >>> <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