RFR: 8230466: check malloc/calloc results in jdk.hotspot.agent

[Yasumasa Suenaga]

Looks good!

Yasumasa (ysuenaga)


On 2019/09/04 16:28, Baesken, Matthias wrote:
> Hello  Yasumasa and Chris, thanks for your input .
> Here is a new  webrev  , without  the unneeded  memset-calls  after calloc .
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/
> 
> Hope everyone is happy with this now �� !
> 
> Best regards, Matthias
> 
> 
>> Hi Matthias,
>>
>> src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:
>> ```
>>    405       // guarantee(symtab == NULL, "multiple symtab");
>>    406       symtab = (struct symtab*)calloc(1, sizeof(struct symtab));
>>    407       if (symtab == NULL) {
>>    408          goto quit;
>>    409       }
>>    410       memset(symtab, 0, sizeof(struct symtab));
>> ```
>>
>> Why do you call memset() to clear symtab in L410?
>> symtab is allocated via calloc() in L406, so symtab would already cleared.
>>
>>
>> Thanks,
>>
>> Yasumasa (ysuenaga)
>>
>>
>> On 2019/09/03 18:14, David Holmes wrote:
>>> Hi Matthias,
>>>
>>> Re-directing to serviceability-dev.
>>>
>>> David
>>>
>>> On 3/09/2019 5:42 pm, Baesken, Matthias wrote:
>>>> Hello, please review the following small fix .
>>>>
>>>> In   jdk.hotspot.agent  native code (linux / macosx)   we miss to check the
>> result of malloc/calloc a few times .
>>>> This should be  adjusted.
>>>> Additionally  I added initialization  to the symtab  array  in  symtab.c   (by
>> calling memset  to make sure we have a defined state )  .
>>>>
>>>>
>>>>
>>>> One question (was not really sure about this one so I did not change it so
>> far) :
>>>>
>>>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.0/src/jdk.hotspot.
>> agent/macosx/native/libsaproc/symtab.c.frames.html
>>>>
>>>> 359 void destroy_symtab(symtab_t* symtab) {
>>>> 360   if (!symtab) return;
>>>> 361   free(symtab->strs);
>>>> 362   free(symtab->symbols);
>>>> 363   free(symtab);
>>>> 364 }
>>>>
>>>>
>>>>
>>>> Here we miss to close   symtab->hash_table   (opened by  dbopen) ,  is it
>> needed  (haven't  used dbopen much - maybe someone can comment on
>> this)?
>>>>
>>>>
>>>> bug/webrev :
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8230466
>>>>
>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.0/
>>>>
>>>>
>>>> Thanks and best regards, Matthias
>>>>