hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Miklos Szegedi (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (YARN-6033) Add support for sections in container-executor configuration file
Date Thu, 03 Aug 2017 21:20:00 GMT

    [ https://issues.apache.org/jira/browse/YARN-6033?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16113478#comment-16113478
] 

Miklos Szegedi edited comment on YARN-6033 at 8/3/17 9:19 PM:
--------------------------------------------------------------

Thank you, [~vvasudev], [~sunilg] and [~wangda]. I looked through the latest patch and found
the following issues. I see one important below, the others seem to be benign or style comments
only.
free_section: Note: It is a good practice to set all released pointers to NULL.
{code}
51      if (section->size > 0) {
52        free(section->kv_pairs);
53      }
{code}
Note: Using a condition {{section->kv_pairs != NULL}} is safer.
{{char *dir = strdup(file_name);}} Note: It does not check for a NULL return
{{size_t linesize = 1000;}} Note: This may be too small for Docker whitelist.
{{if (strlen(line) == 0) {}} Note: …or just line[0]==0 BTW you use it again in the same
function walking through the string again.
{code}
278      len = strlen(line);
279      buffer = (char *) calloc(len + 1, sizeof(char));
280      strncpy(buffer, line, len);
{code}
How about strdup?
{code}
300      if (equaltok == NULL) {
301        // this can happen because no value was set
302        // e.g. banned.users=#this is a comment
303        if (strstr(line, splitter) == NULL) {
304          fprintf(ERRORFILE, "configuration tokenization failed, error with line %s\n",
line);
305          return -1;
306        }
307      }
{code}
Should not we free some memory before {{return -1;}}?
{code}
442      if (free_second_section) {
443        free(section2->name);
444        free(section2);
445      }
{code}
Important: Probably is a good idea to do a memset(section2, 0, sizeof(section2)); here to
avoid some debugging nightmare. The pointers point to freed memory but valid data for now
(which can be allocated and rewritten by some junk later on) and kv pairs attached to another
section, so would not another run of {{get_configuration_section}} pick them up?
{code}
464      if (conf_file == NULL) {
465        fprintf(ERRORFILE, "Invalid conf file provided, unable to open file"
466            " : %s \n", file_path);
467        return (INVALID_CONFIG_FILE);
468      }
{code}
Note: cfg_sections is not freed, if the open fails. I suggest allocating any memory after
this passes.
{code}
  cfg->sections[cfg->size]->name =
473          (char *) calloc(strlen("") + 1, sizeof(char));
474      strncpy(cfg->sections[cfg->size]->name, "", strlen(""));
{code}
Note: You can write this, or just {{cfg->sections[cfg->size]->name = strdup("");}}
{code}
497        if ((cfg->size + 1) % MAX_SIZE == 0) {
498          cfg->sections = (struct section **) realloc(cfg->sections,
499                             sizeof(struct sections *) * (MAX_SIZE + cfg->size));
500          if (cfg->sections == NULL) {
501            fprintf(ERRORFILE,
502                    "Failed re-allocating memory for configuration items\n");
503            exit(OUT_OF_MEMORY);
265          }    504          }
266        }    505        }
{code}
Note: This may leak memory. It should only be done if {{cfg->sections[cfg->size]}}


was (Author: miklos.szegedi@cloudera.com):
Thank you, [~vvasudev] and [~wangda]. I looked through the latest patch and found the following
issues. I see one important below, the others seem to be benign or style comments only.
free_section: Note: It is a good practice to set all released pointers to NULL.
{code}
51      if (section->size > 0) {
52        free(section->kv_pairs);
53      }
{code}
Note: Using a condition {{section->kv_pairs != NULL}} is safer.
{{char *dir = strdup(file_name);}} Note: It does not check for a NULL return
{{size_t linesize = 1000;}} Note: This may be too small for Docker whitelist.
{{if (strlen(line) == 0) {}} Note: …or just line[0]==0 BTW you use it again in the same
function walking through the string again.
{code}
278      len = strlen(line);
279      buffer = (char *) calloc(len + 1, sizeof(char));
280      strncpy(buffer, line, len);
{code}
How about strdup?
{code}
300      if (equaltok == NULL) {
301        // this can happen because no value was set
302        // e.g. banned.users=#this is a comment
303        if (strstr(line, splitter) == NULL) {
304          fprintf(ERRORFILE, "configuration tokenization failed, error with line %s\n",
line);
305          return -1;
306        }
307      }
{code}
Should not we free some memory before {{return -1;}}?
{code}
442      if (free_second_section) {
443        free(section2->name);
444        free(section2);
445      }
{code}
Important: Probably is a good idea to do a memset(section2, 0, sizeof(section2)); here to
avoid some debugging nightmare. The pointers point to freed memory but valid data for now
(which can be allocated and rewritten by some junk later on) and kv pairs attached to another
section, so would not another run of {{get_configuration_section}} pick them up?
{code}
464      if (conf_file == NULL) {
465        fprintf(ERRORFILE, "Invalid conf file provided, unable to open file"
466            " : %s \n", file_path);
467        return (INVALID_CONFIG_FILE);
468      }
{code}
Note: cfg_sections is not freed, if the open fails. I suggest allocating any memory after
this passes.
{code}
  cfg->sections[cfg->size]->name =
473          (char *) calloc(strlen("") + 1, sizeof(char));
474      strncpy(cfg->sections[cfg->size]->name, "", strlen(""));
{code}
Note: You can write this, or just {{cfg->sections[cfg->size]->name = strdup("");}}
{code}
497        if ((cfg->size + 1) % MAX_SIZE == 0) {
498          cfg->sections = (struct section **) realloc(cfg->sections,
499                             sizeof(struct sections *) * (MAX_SIZE + cfg->size));
500          if (cfg->sections == NULL) {
501            fprintf(ERRORFILE,
502                    "Failed re-allocating memory for configuration items\n");
503            exit(OUT_OF_MEMORY);
265          }    504          }
266        }    505        }
{code}
Note: This may leak memory. It should only be done if {{cfg->sections[cfg->size]}}

> Add support for sections in container-executor configuration file
> -----------------------------------------------------------------
>
>                 Key: YARN-6033
>                 URL: https://issues.apache.org/jira/browse/YARN-6033
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>            Reporter: Varun Vasudev
>            Assignee: Varun Vasudev
>         Attachments: YARN-6033.003.patch, YARN-6033.004.patch, YARN-6033.005.patch, YARN-6033.006.patch,
YARN-6033.007.patch, YARN-6033-YARN-5673.001.patch, YARN-6033-YARN-5673.002.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message