RFR(xs): (jdk10) 8173828: realpath is unsafe
Dmitry Samersoff
dmitry.samersoff at oracle.com
Thu Mar 16 05:36:20 UTC 2017
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