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

[David Holmes]

Hi Thomas,

Generally looks good. A few minor comments below.

On 15/03/2017 12:36 AM, Thomas Stüfe wrote:
> Hi Dmitry,
>
> new webrev:
> http://cr.openjdk.java.net/~stuefe/webrevs/8173828-realpath-is-unsafe/jdk10-webrev.02/webrev/

os_solaris.cpp:

+   char* rp = NULL;

This new local variable seems unused.

---

src/os/posix/vm/os_posix.hpp

The comments are a little confusing:

+   // On success, it will return a pointer to the input buffer.
+   // On error, it will return NULL and set errno. Content of output 
buffer is undefined.
+   // On truncation error (output buffer too small), it will return 
NULL and set errno to ENAMETOOLONG.

Is it an input buffer or an output buffer? I think we are inputting an 
output buffer :) Perhaps:

// On success, returns 'outbuf', which now contains the path.
// On error, it will return NULL and set errno. The content of 'outbuf' 
is undefined.
// On truncation error ( 'outbuf' too small), it will return NULL and 
set errno to ENAMETOOLONG.

---

src/os/posix/vm/os_posix.cpp

Grammatical nit:

+     // that it complains about the NULL we did hand down as user buffer.

change to

+     // that it complains about the NULL we handed down as the user buffer.

Nit:

+     // a memory overwriter.
+         guarantee(outbuf[outbuflen - 1] == '\0', "realpath buffer 
overwriter detected.");

overwiter -> overwite

---

Thanks,
David
-----


> Fixed both issues. Note that os::jvm_path() does not have a way to signal
> errors back to the caller. os::jvm_path has a number of issues and could
> benefit from a cleanup, but this is outside the scope of this fix.
>
> Thanks, Thomas
>
>
> On Tue, Mar 14, 2017 at 12:30 PM, Dmitry Samersoff <
> dmitry.samersoff at oracle.com> wrote:
>
>> Thomas,
>>
>>> thank you for the review. These are good points, I addressed them all:
>>
>> os_posix.cpp:
>>
>>   1110: it's better to write as outbuf == NULL
>>
>> os_solaris.cpp:
>>
>>   2037: return value of realpath is not checked. Could you fix it as well?
>>
>> -Dmitry
>>
>>
>> On 2017-03-14 14:19, Thomas Stüfe wrote:
>>> Hi Dmitry,
>>>
>>> thank you for the review. These are good points, I addressed them all:
>>>
>>> New
>>> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
>> realpath-is-unsafe/jdk10-webrev.01/webrev/
>>> Delta to
>>> last: http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
>> realpath-is-unsafe/jdk10-webrev.00-to-01/webrev/
>>>
>>> Thanks, Thomas
>>>
>>> On Tue, Mar 14, 2017 at 10:00 AM, Dmitry Samersoff
>>> <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>>
>> wrote:
>>>
>>>     Thomas,
>>>
>>>     os_posix.cpp:
>>>
>>>     1121: It might be better to return ENAMETOOLONG
>>>
>>>     1131: It might be better to check for
>>>           filename != NULL && outbuf != NULL before any call to realpath
>>>           and return EINVAL for product and assert it for debug build.
>>>
>>>     1134: What is the goal of this assert? Do you see this problem?
>>>
>>>     1135: If realpath overwrites outbuf, strlen might have unpredictable
>>>           effect.
>>>
>>>           So it might be better to set last byte of outbuf to 0 before
>>>           call to realpath and check that it is still zero after call
>>>
>>>     -Dmitry
>>>
>>>     On 2017-03-14 10:11, Thomas Stüfe wrote:
>>>     > Hi all,
>>>     >
>>>     > may I have reviews for this smallish fix.
>>>     >
>>>     > Issue: https://bugs.openjdk.java.net/browse/JDK-8173828
>>>     <https://bugs.openjdk.java.net/browse/JDK-8173828>
>>>     > Webrev:
>>>     >
>>>     http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
>> realpath-is-unsafe/jdk10-webrev.00/
>>>     <http://cr.openjdk.java.net/~stuefe/webrevs/8173828-
>> realpath-is-unsafe/jdk10-webrev.00/>
>>>     >
>>>     > In short, realpath(3) is unsafe the way it is traditionally used
>>>     (with a
>>>     > user buffer provided). It is safe if used in the new POSIX.1-2008
>>>     compliant
>>>     > way. To wrap this behavior, I added a new os::Posix::realpath()
>>>     function
>>>     > which takes a buffer and a buffer size (like a sane API would but
>> the
>>>     > ancient realpath() did not) and moved safe buffer handling into
>>>     this API.
>>>     >
>>>     > Kind Regards, Thomas
>>>     >
>>>
>>>
>>>     --
>>>     Dmitry Samersoff
>>>     Oracle Java development team, Saint Petersburg, Russia
>>>     * I would love to change the world, but they won't give me the
>> sources.
>>>
>>>
>>
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.
>>