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

[serguei.spitsyn at oracle.com]

Hi Matthias,


On 9/5/19 01:19, Baesken, Matthias wrote:
> Hello  Serguei,   thanks  for the comments .
>
>> 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.
>>
> At   line  286  of   build_id_to_debug_filename    we now  return NULL  (in case malloc cannot alloc memory).  So we should check this also
>    at   line 314 , or do I miss something ?

Oh, sorry.
You are right, thanks!

>> 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?
>>
> Good point, I think  we better  add   DeleteLocalRef   here in case of the new  early returns .
>
>
>> 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
>>
>>
> That's a good idea . I added a message .
>
> See  new  webrev :    http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.2/

Thank you for the update!
It looks good to me.

Thanks,
Serguei

>
>
> Thanks, Matthias
>