RFR(xs): (jdk10) 8173828: realpath is unsafe
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Mar 15 10:15:55 UTC 2017
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.
>> > >>
>>
>>
>>
More information about the hotspot-runtime-dev
mailing list