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

[Chris Plummer]

I actually took a look at it the other day but never responded. I was 
wondering if we really want to print a message here. I didn't see any 
other cases of doing this. Also, if we are out of native memory, do we 
really want to continue?

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