RFR(xs): (jdk10) 8173828: realpath is unsafe
David Holmes
david.holmes at oracle.com
Thu Mar 16 01:39:47 UTC 2017
Looks good.
David
On 15/03/2017 8:15 PM, 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
> <mailto: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/
> <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
> <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>
> <mailto: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->
> <mailto:hotspot-runtime-dev- <mailto:hotspot-runtime-dev->>
> > bounces at openjdk.java.net <mailto:bounces at openjdk.java.net>
> <mailto: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>
> <mailto:thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>>>; 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>>>
> > Cc: hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> <mailto: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->
>
> <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>
> <mailto: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->
> <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->
> <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 oracle.com>
> <mailto:dmitry.samersoff at oracle.com
> <mailto:dmitry.samersoff at oracle.com>>
> > <mailto: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>>
> > >>> <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->
> <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->
> <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.
> > >>
>
>
>
More information about the hotspot-runtime-dev
mailing list