RFR(xs): (jdk10) 8173828: realpath is unsafe
David Holmes
david.holmes at oracle.com
Wed Mar 15 01:18:03 UTC 2017
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.
>>
More information about the hotspot-runtime-dev
mailing list