Quantcast

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

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi all,

may I have reviews for this smallish fix.

Issue: https://bugs.openjdk.java.net/browse/JDK-8173828
Webrev:
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Samersoff
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
> Webrev:
> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
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 <
[hidden email]> 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
> > Webrev:
> > 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.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Samersoff
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
> <[hidden email] <mailto:[hidden email]>> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Dmitry,

new webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.02/webrev/

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 <
[hidden email]> 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
> > <[hidden email] <mailto:[hidden email]>>
> 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.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Samersoff
Thomas,

Looks good to me. Thank you for fixing it!

-Dmitry


On 2017-03-14 17:36, Thomas Stüfe wrote:

> Hi Dmitry,
>
> new
> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.02/webrev/
>
> 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
> <[hidden email] <mailto:[hidden email]>> 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/
>     <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/
>     <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
>     > <[hidden email] <mailto:[hidden email]>
>     <mailto:[hidden email]
>     <mailto:[hidden email]>>> 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-realpath-is-unsafe/jdk10-webrev.00/
>     <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/
>     <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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Thanks for the review!

On Tue, Mar 14, 2017 at 4:20 PM, Dmitry Samersoff <
[hidden email]> wrote:

> Thomas,
>
> Looks good to me. Thank you for fixing it!
>
> -Dmitry
>
>
> On 2017-03-14 17:36, Thomas Stüfe wrote:
> > Hi Dmitry,
> >
> > new
> > webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
> realpath-is-unsafe/jdk10-webrev.02/webrev/
> >
> > 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
> > <[hidden email] <mailto:[hidden email]>>
> 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/
> >     <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/
> >     <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
> >     > <[hidden email] <mailto:[hidden email]>
> >     <mailto:[hidden email]
> >     <mailto:[hidden email]>>> 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-
> realpath-is-unsafe/jdk10-webrev.00/
> >     <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/
> >     <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.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by Thomas Stüfe-2
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 <
> [hidden email]> 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
>>> <[hidden email] <mailto:[hidden email]>>
>> 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.
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: 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-
> [hidden email]] On Behalf Of David Holmes
> Sent: Mittwoch, 15. März 2017 02:18
> To: Thomas Stüfe <[hidden email]>; Dmitry Samersoff
> <[hidden email]>
> Cc: [hidden email]
> 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 <
> > [hidden email]> 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
> >>> <[hidden email]
> <mailto:[hidden email]>>
> >> 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.
> >>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
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

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.

Kind Regards, Thomas




On Wed, Mar 15, 2017 at 8:30 AM, Langer, Christoph <[hidden email]
> 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-
> > [hidden email]] On Behalf Of David Holmes
> > Sent: Mittwoch, 15. März 2017 02:18
> > To: Thomas Stüfe <[hidden email]>; Dmitry Samersoff
> > <[hidden email]>
> > Cc: [hidden email]
> > 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 <
> > > [hidden email]> 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
> > >>> <[hidden email]
> > <mailto:[hidden email]>>
> > >> 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.
> > >>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Samersoff
Thomas,

> Interesting that no-one commented on the
> guarantee in case of an overwrite, you are all ok with it?

Personally, I like gurantee() here.

-Dmitry

On 2017-03-15 11:41, 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
>
> 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.
>
> Kind Regards, Thomas
>
>
>
>
> On Wed, Mar 15, 2017 at 8:30 AM, Langer, Christoph
> <[hidden email] <mailto:[hidden email]>> 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->
>     > [hidden email] <mailto:[hidden email]>] On
>     Behalf Of David Holmes
>     > Sent: Mittwoch, 15. März 2017 02:18
>     > To: Thomas Stüfe <[hidden email]
>     <mailto:[hidden email]>>; Dmitry Samersoff
>     > <[hidden email] <mailto:[hidden email]>>
>     > Cc: [hidden email]
>     <mailto:[hidden email]>
>     > 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 <
>     > > [hidden email]
>     <mailto:[hidden email]>> 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
>     > >>> <[hidden email] <mailto:[hidden email]>
>     > <mailto:[hidden email]
>     <mailto:[hidden email]>>>
>     > >> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by Thomas Stüfe-2
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
> <[hidden email] <mailto:[hidden email]>> 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->
>     > [hidden email] <mailto:[hidden email]>] On
>     Behalf Of David Holmes
>     > Sent: Mittwoch, 15. März 2017 02:18
>     > To: Thomas Stüfe <[hidden email]
>     <mailto:[hidden email]>>; Dmitry Samersoff
>     > <[hidden email] <mailto:[hidden email]>>
>     > Cc: [hidden email]
>     <mailto:[hidden email]>
>     > 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 <
>     > > [hidden email]
>     <mailto:[hidden email]>> 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
>     > >>> <[hidden email] <mailto:[hidden email]>
>     > <mailto:[hidden email]
>     <mailto:[hidden email]>>>
>     > >> 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.
>     > >>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
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 <[hidden email]>
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
>> <[hidden email] <mailto:[hidden email]>> 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->
>>     > [hidden email] <mailto:[hidden email]>] On
>>     Behalf Of David Holmes
>>     > Sent: Mittwoch, 15. März 2017 02:18
>>     > To: Thomas Stüfe <[hidden email]
>>     <mailto:[hidden email]>>; Dmitry Samersoff
>>     > <[hidden email] <mailto:[hidden email]>>
>>     > Cc: [hidden email]
>>     <mailto:[hidden email]>
>>
>>     > 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 <
>>     > > [hidden email]
>>     <mailto:[hidden email]>> 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
>>     > >>> <[hidden email] <mailto:dmitry.samersoff@oracl
>> e.com>
>>     > <mailto:[hidden email]
>>
>>     <mailto:[hidden email]>>>
>>     > >> 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.
>>     > >>
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
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 <[hidden email]
> <mailto:[hidden email]>> 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
>         <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>> 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->>
>             > [hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>] On
>             Behalf Of David Holmes
>             > Sent: Mittwoch, 15. März 2017 02:18
>             > To: Thomas Stüfe <[hidden email]
>         <mailto:[hidden email]>
>             <mailto:[hidden email]
>         <mailto:[hidden email]>>>; Dmitry Samersoff
>             > <[hidden email]
>         <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>>
>             > Cc: [hidden email]
>         <mailto:[hidden email]>
>             <mailto:[hidden email]
>         <mailto:[hidden email]>>
>
>             > 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 <
>             > > [hidden email]
>         <mailto:[hidden email]>
>             <mailto:[hidden email]
>         <mailto:[hidden email]>>> 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
>             > >>> <[hidden email]
>         <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>
>             > <mailto:[hidden email]
>         <mailto:[hidden email]>
>
>             <mailto:[hidden email]
>         <mailto:[hidden email]>>>>
>             > >> 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.
>             > >>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Samersoff
In reply to this post by Thomas Stüfe-2
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 <[hidden email]>
> 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
>>> <[hidden email] <mailto:[hidden email]>> 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->
>>>     > [hidden email] <mailto:[hidden email]>] On
>>>     Behalf Of David Holmes
>>>     > Sent: Mittwoch, 15. März 2017 02:18
>>>     > To: Thomas Stüfe <[hidden email]
>>>     <mailto:[hidden email]>>; Dmitry Samersoff
>>>     > <[hidden email] <mailto:[hidden email]>>
>>>     > Cc: [hidden email]
>>>     <mailto:[hidden email]>
>>>
>>>     > 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 <
>>>     > > [hidden email]
>>>     <mailto:[hidden email]>> 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
>>>     > >>> <[hidden email] <mailto:dmitry.samersoff@oracl
>>> e.com>
>>>     > <mailto:[hidden email]
>>>
>>>     <mailto:[hidden email]>>>
>>>     > >> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Chris, David,

Can one of you sponsor this change? Thanks!

..Thomas

On Thu, Mar 16, 2017 at 6:36 AM, Dmitry Samersoff <
[hidden email]> 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/
> >
> > Kind Regards, Thomas
> >
> >
> > On Wed, Mar 15, 2017 at 11:02 AM, David Holmes <[hidden email]>
> > 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
> >>> <[hidden email] <mailto:[hidden email]>> 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->
> >>>     > [hidden email] <mailto:[hidden email]>] On
> >>>     Behalf Of David Holmes
> >>>     > Sent: Mittwoch, 15. März 2017 02:18
> >>>     > To: Thomas Stüfe <[hidden email]
> >>>     <mailto:[hidden email]>>; Dmitry Samersoff
> >>>     > <[hidden email] <mailto:[hidden email]
> >>
> >>>     > Cc: [hidden email]
> >>>     <mailto:[hidden email]>
> >>>
> >>>     > 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 <
> >>>     > > [hidden email]
> >>>     <mailto:[hidden email]>> 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
> >>>     > >>> <[hidden email] <mailto:dmitry.samersoff@oracl
> >>> e.com>
> >>>     > <mailto:[hidden email]
> >>>
> >>>     <mailto:[hidden email]>>>
> >>>     > >> 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.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
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
> <[hidden email] <mailto:[hidden email]>> 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
>     <[hidden email] <mailto:[hidden email]>>
>     > 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
>     >>> <[hidden email] <mailto:[hidden email]>
>     <mailto:[hidden email] <mailto:[hidden email]>>>
>     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->>
>     >>>     > [hidden email] <mailto:[hidden email]>
>     <mailto:[hidden email] <mailto:[hidden email]>>] On
>     >>>     Behalf Of David Holmes
>     >>>     > Sent: Mittwoch, 15. März 2017 02:18
>     >>>     > To: Thomas Stüfe <[hidden email]
>     <mailto:[hidden email]>
>     >>>     <mailto:[hidden email]
>     <mailto:[hidden email]>>>; Dmitry Samersoff
>     >>>     > <[hidden email]
>     <mailto:[hidden email]>
>     <mailto:[hidden email]
>     <mailto:[hidden email]>>>
>     >>>     > Cc: [hidden email]
>     <mailto:[hidden email]>
>     >>>     <mailto:[hidden email]
>     <mailto:[hidden email]>>
>     >>>
>     >>>     > 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 <
>     >>>     > > [hidden email]
>     <mailto:[hidden email]>
>     >>>     <mailto:[hidden email]
>     <mailto:[hidden email]>>> 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
>     >>>     > >>> <[hidden email]
>     <mailto:[hidden email]> <mailto:dmitry.samersoff@oracl
>     <mailto:dmitry.samersoff@oracl>
>     >>> e.com <http://e.com>>
>     >>>     > <mailto:[hidden email]
>     <mailto:[hidden email]>
>     >>>
>     >>>     <mailto:[hidden email]
>     <mailto:[hidden email]>>>>
>     >>>     > >> 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.
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Thank you!

On Thu, Mar 16, 2017 at 9:09 AM, David Holmes <[hidden email]>
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
>> <[hidden email] <mailto:[hidden email]>> 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
>>     <[hidden email] <mailto:[hidden email]>>
>>
>>     > 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
>>     >>> <[hidden email] <mailto:[hidden email]>
>>     <mailto:[hidden email] <mailto:[hidden email]>>>
>>     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->>
>>     >>>     > [hidden email] <mailto:[hidden email]>
>>     <mailto:[hidden email] <mailto:[hidden email]>>]
>> On
>>     >>>     Behalf Of David Holmes
>>     >>>     > Sent: Mittwoch, 15. März 2017 02:18
>>     >>>     > To: Thomas Stüfe <[hidden email]
>>     <mailto:[hidden email]>
>>     >>>     <mailto:[hidden email]
>>     <mailto:[hidden email]>>>; Dmitry Samersoff
>>     >>>     > <[hidden email]
>>     <mailto:[hidden email]>
>>     <mailto:[hidden email]
>>     <mailto:[hidden email]>>>
>>     >>>     > Cc: [hidden email]
>>     <mailto:[hidden email]>
>>     >>>     <mailto:[hidden email]
>>
>>     <mailto:[hidden email]>>
>>     >>>
>>     >>>     > 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 <
>>     >>>     > > [hidden email]
>>     <mailto:[hidden email]>
>>     >>>     <mailto:[hidden email]
>>     <mailto:[hidden email]>>> 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
>>     >>>     > >>> <[hidden email]
>>     <mailto:[hidden email]> <mailto:dmitry.samersoff@oracl
>>     <mailto:dmitry.samersoff@oracl>
>>     >>> e.com <http://e.com>>
>>     >>>     > <mailto:[hidden email]
>>     <mailto:[hidden email]>
>>
>>     >>>
>>     >>>     <mailto:[hidden email]
>>     <mailto:[hidden email]>>>>
>>     >>>     > >> 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.
>>
>>
>>
Loading...