RFR(xs): (jdk10) 8173828: realpath is unsafe
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Mar 16 09:15:10 UTC 2017
Thank you!
On Thu, Mar 16, 2017 at 9:09 AM, David Holmes <david.holmes at oracle.com>
wrote:
> I'll take care of it.
>
> David
>
> On 16/03/2017 5:24 PM, Thomas Stüfe wrote:
>
>> 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 <mailto: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/
>> <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-
>> <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-
>> <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 oracl
>> <mailto:dmitry.samersoff at oracl>
>> >>> e.com <http://e.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.
>> >>> > >>
>> >>>
>> >>>
>> >>>
>>
>>
>> --
>> 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