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

[Chris Plummer]

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,

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
>>>>>>
>>>>
>>>
>>>
>>
>