RFR(xs): (jdk10) 8173828: realpath is unsafe

[Langer, Christoph]

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-
> 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>; Dmitry Samersoff
> <dmitry.samersoff at oracle.com>
> Cc: 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-
> 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> 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-
> >> realpath-is-unsafe/jdk10-webrev.01/webrev/
> >>> Delta to
> >>> last: 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>>
> >> 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>
> >>>     > Webrev:
> >>>     >
> >>>     http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
> >> realpath-is-unsafe/jdk10-webrev.00/
> >>>     <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.
> >>