RFR (XS): 8175510 Null pointer dereference in getModuleObject of JPLISAgent.c:790

[serguei.spitsyn at oracle.com]

On 10/18/17 13:33, Chris Plummer wrote:
> On 10/18/17 1:10 PM, serguei.spitsyn at oracle.com wrote:
>> On 10/18/17 13:02, serguei.spitsyn at oracle.com wrote:
>>> Hi Chris,
>>>
>>>
>>> On 10/18/17 12:34, Chris Plummer wrote:
>>>> I actually took a look at it the other day but never responded.
>>>
>>> Thank you for reviewing it!
>>>
>>>> I was wondering if we really want to print a message here.I didn't 
>>>> see any other cases of doing this.
>>>
>>> I see no harm to print it in this particular case at least to have 
>>> some clue about what is going on.
>>>
>>>
>>>> Also, if we are out of native memory, do we really want to continue?
>>>
>>> In this case, the VM itself will be aborted with a big probability.
>>> I do not see cases where the JDWP agent is aborted other than at 
>>> initialization.
>>
>> Sorry for the typo, I had to say the JPLIS agent. :)
> Ok. Changes are fine.

Thanks a lot, Chris!
Serguei

>
> thanks,
>
> Chris
>>
>> Thanks,
>> Serguei
>>
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>>
>>>> Chris
>>>>
>>>> On 10/18/17 12:15 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Is anyone interested to review this simple fix?
>>>>> Otherwise, I'd suggest to push it with one review from David under 
>>>>> trivial fix rule.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 10/12/17 18:01, David Holmes wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> Seems quite reasonable.
>>>>>>
>>>>>> Reviewed.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 13/10/2017 8:21 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>> Please, review a fix for the Parfait bug:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8175510
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8175510-jplis-parfait.1/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Summary:
>>>>>>>
>>>>>>>    This is the main fragment from the Parfait report:
>>>>>>>
>>>>>>>
>>>>>>>         getModuleObject
>>>>>>>
>>>>>>> FileExpandCollapseLine 
>>>>>>> #jdk/src/java.instrument/share/native/libinstrument/JPLISAgent.c
>>>>>>> jdk-9+180-JDK9_linux
>>>>>>>
>>>>>>> 783.
>>>>>>>
>>>>>>>      int len = (last_slash == NULL) ? 0 : (int)(last_slash - 
>>>>>>> cname);
>>>>>>>
>>>>>>> 784.
>>>>>>>
>>>>>>>      char* pkg_name_buf = (char*)malloc(len + 1);
>>>>>>>
>>>>>>> 785.
>>>>>>> 786.
>>>>>>>
>>>>>>>      jplis_assert_msg(pkg_name_buf != NULL, "OOM error in native 
>>>>>>> tmp buffer allocation");
>>>>>>>
>>>>>>> Pointer checked against constant 'NULL' but does not protect the 
>>>>>>> dereference.
>>>>>>> 787.
>>>>>>>
>>>>>>>      if (last_slash != NULL) {
>>>>>>>
>>>>>>> 788.
>>>>>>>
>>>>>>>          strncpy(pkg_name_buf, cname, len);
>>>>>>>
>>>>>>> 789.
>>>>>>>
>>>>>>>      }
>>>>>>>
>>>>>>> 790.
>>>>>>>
>>>>>>>      pkg_name_buf[len] = '\0';
>>>>>>>
>>>>>>> *Null pointer dereference not protected by null check*
>>>>>>> Write to pointer pkg_name_buf that could be constant 'NULL'
>>>>>>>
>>>>>>>
>>>>>>>    The malloc can return NULL in a case of OOME.
>>>>>>>    The assert at L786 checks the returned pointer for NULL but 
>>>>>>> does not protect the dereference at L790.
>>>>>>>    The fix is to replace the assert with printing a error 
>>>>>>> message and returning with NULL from the getModuleObject().
>>>>>>>    It must be safe as the returned result is passed to the 
>>>>>>> sun.instrument.InstrumentationImpl.transform()
>>>>>>>    which handles null passed as in the module parameter.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>