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

[serguei.spitsyn at oracle.com]

Hi Matthias,


It looks good in general but I have some minor comments below.


http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c.frames.html

  279 build_id_to_debug_filename (size_t size, unsigned char *data)
  280 {
  . . .
  283   filename = malloc(strlen (debug_file_directory) + (sizeof 
"/.build-id/" - 1) + 1
  284                     + 2 * size + (sizeof ".debug" - 1) + 1);
  285   if (filename == NULL) {
  286     return NULL;
  287   }
  . . .
  312   char *filename
  313     = (build_id_to_debug_filename (note->n_descsz, bytes));
  314   if (filename == NULL) {
  315     return NULL;
  316   }

There is no need to check filename for NULL at the line 314 as the function
build_id_to_debug_filename with new check at the line 285 never returns 
NULL.


http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m.frames.html

  354   array = (*env)->NewByteArray(env, numBytes);
  . . .
  376   if (pages == NULL) {
  377     return NULL;
  378   }
  379   mapped = calloc(pageCount, sizeof(int));
  380   if (mapped == NULL) {
  381     free(pages);
  382     return NULL;
  383   }

Just a question:
   We do not release the array allocated at line 354 because this local 
reference
   will be auto-released when returning to Java. Is this correct?


http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/src/jdk.hotspot.agent/macosx/native/libsaproc/symtab.c.frames.html

   69     if (is_debug()) {
   70       DBT rkey, rvalue;  71       char* tmp = (char 
*)malloc(strlen(symtab->symbols[i].name) + 1);
   72       if (tmp != NULL) {
   73         strcpy(tmp, symtab->symbols[i].name);
   74         rkey.data = tmp;
   75         rkey.size = strlen(tmp) + 1;
   76 (*symtab->hash_table->get)(symtab->hash_table, &rkey, &rvalue, 0);
   77         // we may get a copy back so compare contents
   78         symtab_symbol *res = (symtab_symbol *)rvalue.data;
   79         if (strcmp(res->name, symtab->symbols[i].name) ||
   80           res->offset != symtab->symbols[i].offset ||
   81           res->size != symtab->symbols[i].size) {
   82             print_debug("error to get hash_table value!\n");
   83         }
   84         free(tmp);
   85       }

If malloc returns NULL then this debugging part will be we silently skipped.
In other such cases there is an attempt to print a debug message.
For instance:

  140   symtab = (symtab_t *)malloc(sizeof(symtab_t));
  141   if (symtab == NULL) {
  142     print_debug("out of memory: allocating symtab\n");
  143     return NULL;
  144   }

I understand that print_debug can fail with out of memory as well.
But it depends on its implementation.

Thanks,
Serguei




On 9/4/19 00: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
>>>>