RFR: 8229378: jdwp library loader in linker_md.c quietly truncates on buffer overflow

Mon Aug 26 13:00:06 UTC 2019

I haven't reviewed the change. I just noticed that we needed info on how
the fix was tested.

Dan

On 8/26/19 3:27 AM, serguei.spitsyn at oracle.com wrote:
> Hi Adam,
>
> Thank you for the explanation below!
> Then I'm Okay with the fix as it is.
>
> Dan,
>
> Do you have any suggestions or objections?
> If not, then do I need to add your name to the list of reviewers?
>
> Thanks,
> Serguei
>
>
> On 8/15/19 04:38, Adam Farley8 wrote:
>> Hi Serguei, Daniel,
>>
>> Good to hear you like the fix.
>>
>> My intention with the testing was to make sure my change didn't break 
>> anything else. I didn't do a code paths check before I ran it though; 
>> saturation run.
>>
>> As for writing a new test, I'm finding it tricky.
>>
>> Here's the current flow:
>>
>> Step 1) VM initialises.
>> Step 2) VM loads a couple of libraries and shuts down if one or more 
>> paths is too long in sun.boot.library.path.
>> Step 3) JDWP initializes
>> Step 4) JDWP loads a library and shuts down if one or more paths is 
>> too long in sun.boot.library.path.
>>
>> As you can see, Step 2 prevents us from reaching Step 4 with a 
>> too-long-path (required to cause failure).
>>
>> I worked around that with my webrev by disabling the bit in os.cpp 
>> that enacts Step 2.
>>
>> Since my hack will be removed in the final webrev, we need another 
>> way to reach step 4.
>>
>> So what we need to test this change, I believe, is a way to insert 
>> Step 2.5) Change the property to include a too-long path.
>>
>> This allows the VM to start up properly, but gives us the excessive 
>> path we need to test the jdwp fix.
>>
>> Right now, I'm not seeing a way to do this outside of using the JNI.
>>
>> 1) shell script launches cpp file.
>> 2) cpp starts vm without jdwp.
>> 3) change the property.
>> 4) call jdwp library-loading method directly.
>> 5) check the return code.
>>
>> This seems messy, but I'm not seeing a way to initialise jdwp from 
>> inside java code (which sounds better to me).
>>
>> I welcome anyone who can think of a better way to do this.
>>
>> Best Regards
>>
>> Adam Farley
>> IBM Runtimes
>>
>>
>> "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote on 
>> 15/08/2019 09:25:36:
>>
>> > From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> > To: Adam Farley8 <adam.farley at uk.ibm.com>
>> > Cc: Chris Plummer <chris.plummer at oracle.com>,
>> > daniel.daugherty at oracle.com, serviceability-dev at openjdk.java.net
>> > Date: 15/08/2019 09:25
>> > Subject: Re: RFR: 8229378: jdwp library loader in linker_md.c
>> > quietly truncates on buffer overflow
>> > 
>> > Hi Adam,
>> >
>> > The fix itself looks Okay to me.
>> > I'm not sure there is any test case in these test suites which
>> > provide a coverage for it.
>> > It looks like you need to develop a unit jtreg unit test for this.
>> >
>> > Thanks,
>> > Serguei
>> >
>> >
>> > On 8/13/19 09:28, Adam Farley8 wrote:
>> > Hi Serguei, Daniel, 
>> >
>> > My testing was limited to the bug specific test case I mentioned,
>> > and the following jdwp tests:
>> >
>> > test/jdk/com/sun/jdi/Jdwp*
>> > test/hotspot/jtreg/serviceability/jdwp
>> >
>> > Best Regards
>> >
>> > Adam Farley
>> > IBM Runtimes
>> >
>> >
>> > "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote on
>> > 13/08/2019 17:04:43:
>> >
>> > > From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> > > To: daniel.daugherty at oracle.com, Adam Farley8
>> > > <adam.farley at uk.ibm.com>, Chris Plummer <chris.plummer at oracle.com>
>> > > Cc: serviceability-dev at openjdk.java.net
>> > > Date: 13/08/2019 17:08
>> > > Subject: Re: RFR: 8229378: jdwp library loader in linker_md.c
>> > > quietly truncates on buffer overflow
>> > >
>> > > Hi Adam,
>> > >
>> > > I'm looking at your fix.
>> > > Also interested about your testing.
>> > >
>> > > Thanks,
>> > > Serguei
>> > >
>> > > On 8/13/19 08:48, Daniel D. Daugherty wrote:
>> > > I don't see any information about how this change was tested...
>> > > Is there something on another email thread?
>> > >
>> > > Dan
>> > >
>> >
>> > > On 8/13/19 11:41 AM, Adam Farley8 wrote:
>> > > Hi Chris,
>> > >
>> > > Thanks!
>> > >
>> > > I understand we need a second reviewer/sponsor to get this change
>> > > in. Any volunteers?
>> > >
>> > > Best Regards
>> > >
>> > > Adam Farley
>> > > IBM Runtimes
>> > >
>> > >
>> > > Chris Plummer <chris.plummer at oracle.com> wrote on 12/08/2019 
>> 21:35:06:
>> > >
>> > > > From: Chris Plummer <chris.plummer at oracle.com>
>> > > > To: Adam Farley8 <adam.farley at uk.ibm.com>, serviceability-
>> > > dev at openjdk.java.net
>> > > > Date: 12/08/2019 21:35
>> > > > Subject: Re: RFR: 8229378: jdwp library loader in linker_md.c
>> > > > quietly truncates on buffer overflow
>> > > >
>> > > > Hi Adam,
>> > > >
>> > > > It looks good to me.
>> > > >
>> > > > thanks,
>> > > >
>> > > > Chris
>> > > >
>> > > > On 8/12/19 7:34 AM, Adam Farley8 wrote:
>> > > > Hi All,
>> > > >
>> > > > This is a known bug, mentioned in a code comment.
>> > > >
>> > > > Here is the fix for that bug.
>> > > >
>> > > > Reviewers and sponsors requested.
>> > > >
>> > > > Short version: if you set sun.boot.library.path to
>> > > > something beyond a system's max path length, the
>> > > > current code will return an empty string (rather than
>> > > > printing a useful error message and shutting down).
>> > > >
>> > > > This is also a problem if you've specified multiple
>> > > > paths with a separator, as this code seems to wrongly
>> > > > assess whether the *total* length exceeds max path
>> > > > length. So two 200 char paths on windows will cause
>> > > > failure, as the total length is 400 (which is beyond
>> > > > max length for windows).
>> > > >
>> > > > Note that the os.cpp bit of the webrev will not be included
>> > > > in the final webrev, it just makes this change trivially
>> > > > testable.
>> > > >
>> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8229378
>> > > > Webrev: http://cr.openjdk.java.net/~afarley/8229378/webrev/
>> > > >
>> > > >
>> > > > Best Regards
>> > > >
>> > > > Adam Farley
>> > > > IBM Runtimes
>> > > >
>> > > > Unless stated otherwise above:
>> > > > IBM United Kingdom Limited - Registered in England and Wales with
>> > > > number 741598.
>> > > > Registered office: PO Box 41, North Harbour, Portsmouth, 
>> Hampshire PO6 3AU
>> > > Unless stated otherwise above:
>> > > IBM United Kingdom Limited - Registered in England and Wales with
>> > > number 741598.
>> > > Registered office: PO Box 41, North Harbour, Portsmouth, 
>> Hampshire PO6 3AU
>> > Unless stated otherwise above:
>> > IBM United Kingdom Limited - Registered in England and Wales with
>> > number 741598.
>> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire 
>> PO6 3AU
>> Unless stated otherwise above:
>> IBM United Kingdom Limited - Registered in England and Wales with 
>> number 741598.
>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire 
>> PO6 3AU
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190826/874cc95a/attachment.html>