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

[Baesken, Matthias]

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 ?


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


Thanks, Matthias