[
https://issues.apache.org/jira/browse/YARN-6033?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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) {
52free(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
303if (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) {
443free(section2->name);
444free(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) {
465fprintf(ERRORFILE, "Invalid conf file provided, unable to open file"
466" : %s \n", file_path);
467return (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}
497if ((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) {
501fprintf(ERRORFILE,
502"Failed re-allocating memory for configuration items\n");
503exit(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.szeg...@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) {
52free(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
303if (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) {
443free(section2->name);
444free(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
