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

Dmitry Samersoff dmitry.samersoff at oracle.com
Tue Mar 14 15:20:36 UTC 2017


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
> <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>> wrote:
> 
>     Thomas,
> 
>     > thank you for the review. These are good points, I addressed them all:
> 
>     os_posix.cpp:
> 
>       1110: it's better to write as outbuf == NULL
> 
>     os_solaris.cpp:
> 
>       2037: return value of realpath is not checked. Could you fix it as
>     well?
> 
>     -Dmitry
> 
> 
>     On 2017-03-14 14:19, Thomas Stüfe wrote:
>     > Hi Dmitry,
>     >
>     > thank you for the review. These are good points, I addressed them all:
>     >
>     > New
>     > webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-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
>     > <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>
>     <mailto:dmitry.samersoff at oracle.com
>     <mailto:dmitry.samersoff at oracle.com>>> wrote:
>     >
>     >     Thomas,
>     >
>     >     os_posix.cpp:
>     >
>     >     1121: It might be better to return ENAMETOOLONG
>     >
>     >     1131: It might be better to check for
>     >           filename != NULL && outbuf != NULL before any call to
>     realpath
>     >           and return EINVAL for product and assert it for debug build.
>     >
>     >     1134: What is the goal of this assert? Do you see this problem?
>     >
>     >     1135: If realpath overwrites outbuf, strlen might have
>     unpredictable
>     >           effect.
>     >
>     >           So it might be better to set last byte of outbuf to 0 before
>     >           call to realpath and check that it is still zero after call
>     >
>     >     -Dmitry
>     >
>     >     On 2017-03-14 10:11, Thomas Stüfe wrote:
>     >     > Hi all,
>     >     >
>     >     > may I have reviews for this smallish fix.
>     >     >
>     >     > Issue: https://bugs.openjdk.java.net/browse/JDK-8173828
>     <https://bugs.openjdk.java.net/browse/JDK-8173828>
>     >     <https://bugs.openjdk.java.net/browse/JDK-8173828
>     <https://bugs.openjdk.java.net/browse/JDK-8173828>>
>     >     > 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.


More information about the hotspot-runtime-dev mailing list