From a4ac21e9a8cfe8a73471a195308a742e07d7fe8d Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Wed, 1 Aug 2018 15:32:38 +0200 Subject: [PATCH 1/3] readConfigFile: assign and check 'key' separately ... to make the code readable. No changes in behavior intended by this commit. --- config.c | 312 +++++++++++++++++++++++++++---------------------------- 1 file changed, 152 insertions(+), 160 deletions(-) diff --git a/config.c b/config.c index 84db36f..d2fba10 100644 --- a/config.c +++ b/config.c @@ -1037,7 +1037,8 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) } if (isalpha((unsigned char)*start)) { - if ((key = isolateWord(&start, &buf, length)) == NULL) + key = isolateWord(&start, &buf, length); + if (key == NULL) continue; if (!strcmp(key, "compress")) { newlog->flags |= LOG_FLAG_COMPRESS; @@ -1191,16 +1192,16 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) } } else if (!strcmp(key, "shredcycles")) { free(key); - if ((key = isolateValue(configFile, lineNum, "shred cycles", - &start, &buf, length)) != NULL) { - newlog->shred_cycles = strtoul(key, &chptr, 0); - if (*chptr || newlog->shred_cycles < 0) { - message(MESS_ERROR, "%s:%d bad shred cycles '%s'\n", - configFile, lineNum, key); - goto error; - } + key = isolateValue(configFile, lineNum, "shred cycles", + &start, &buf, length); + if (key == NULL) + continue; + newlog->shred_cycles = strtoul(key, &chptr, 0); + if (*chptr || newlog->shred_cycles < 0) { + message(MESS_ERROR, "%s:%d bad shred cycles '%s'\n", + configFile, lineNum, key); + goto error; } - else continue; } else if (!strcmp(key, "hourly")) { newlog->criterium = ROT_HOURLY; } else if (!strcmp(key, "daily")) { @@ -1232,59 +1233,53 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) newlog->criterium = ROT_YEARLY; } else if (!strcmp(key, "rotate")) { free(key); - if ((key = isolateValue - (configFile, lineNum, "rotate count", &start, - &buf, length)) != NULL) { - - newlog->rotateCount = strtoul(key, &chptr, 0); - if (*chptr || newlog->rotateCount < 0) { - message(MESS_ERROR, - "%s:%d bad rotation count '%s'\n", - configFile, lineNum, key); - RAISE_ERROR(); - } + key = isolateValue(configFile, lineNum, "rotate count", &start, + &buf, length); + if (key == NULL) + continue; + newlog->rotateCount = strtoul(key, &chptr, 0); + if (*chptr || newlog->rotateCount < 0) { + message(MESS_ERROR, + "%s:%d bad rotation count '%s'\n", + configFile, lineNum, key); + RAISE_ERROR(); } - else continue; } else if (!strcmp(key, "start")) { free(key); - if ((key = isolateValue - (configFile, lineNum, "start count", &start, - &buf, length)) != NULL) { - - newlog->logStart = strtoul(key, &chptr, 0); - if (*chptr || newlog->logStart < 0) { - message(MESS_ERROR, "%s:%d bad start count '%s'\n", - configFile, lineNum, key); - RAISE_ERROR(); - } + key = isolateValue(configFile, lineNum, "start count", &start, + &buf, length); + if (key == NULL) + continue; + newlog->logStart = strtoul(key, &chptr, 0); + if (*chptr || newlog->logStart < 0) { + message(MESS_ERROR, "%s:%d bad start count '%s'\n", + configFile, lineNum, key); + RAISE_ERROR(); } - else continue; } else if (!strcmp(key, "minage")) { free(key); - if ((key = isolateValue - (configFile, lineNum, "minage count", &start, - &buf, length)) != NULL) { - newlog->rotateMinAge = strtoul(key, &chptr, 0); - if (*chptr || newlog->rotateMinAge < 0) { - message(MESS_ERROR, "%s:%d bad minimum age '%s'\n", - configFile, lineNum, start); - RAISE_ERROR(); - } + key = isolateValue(configFile, lineNum, "minage count", &start, + &buf, length); + if (key == NULL) + continue; + newlog->rotateMinAge = strtoul(key, &chptr, 0); + if (*chptr || newlog->rotateMinAge < 0) { + message(MESS_ERROR, "%s:%d bad minimum age '%s'\n", + configFile, lineNum, start); + RAISE_ERROR(); } - else continue; } else if (!strcmp(key, "maxage")) { free(key); - if ((key = isolateValue - (configFile, lineNum, "maxage count", &start, - &buf, length)) != NULL) { - newlog->rotateAge = strtoul(key, &chptr, 0); - if (*chptr || newlog->rotateAge < 0) { - message(MESS_ERROR, "%s:%d bad maximum age '%s'\n", - configFile, lineNum, start); - RAISE_ERROR(); - } + key = isolateValue(configFile, lineNum, "maxage count", &start, + &buf, length); + if (key == NULL) + continue; + newlog->rotateAge = strtoul(key, &chptr, 0); + if (*chptr || newlog->rotateAge < 0) { + message(MESS_ERROR, "%s:%d bad maximum age '%s'\n", + configFile, lineNum, start); + RAISE_ERROR(); } - else continue; } else if (!strcmp(key, "errors")) { message(MESS_DEBUG, "%s: %d: the errors directive is deprecated and no longer used.\n", @@ -1337,48 +1332,48 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) continue; } free(key); - if ((key = isolateValue(configFile, lineNum, "tabooext", &start, - &buf, length)) != NULL) { - endtag = key; - if (*endtag == '+') { + key = isolateValue(configFile, lineNum, "tabooext", &start, + &buf, length); + if (key == NULL) + continue; + endtag = key; + if (*endtag == '+') { + endtag++; + while (isspace((unsigned char)*endtag) && *endtag) endtag++; - while (isspace((unsigned char)*endtag) && *endtag) - endtag++; - } else { - free_2d_array(tabooPatterns, tabooCount); - tabooCount = 0; - /* realloc of NULL is safe by definition */ - tabooPatterns = NULL; - } - - while (*endtag) { - int bytes; - char *pattern = NULL; + } else { + free_2d_array(tabooPatterns, tabooCount); + tabooCount = 0; + /* realloc of NULL is safe by definition */ + tabooPatterns = NULL; + } - chptr = endtag; - while (!isspace((unsigned char)*chptr) && *chptr != ',' && *chptr) - chptr++; + while (*endtag) { + int bytes; + char *pattern = NULL; - /* accept only non-empty patterns to avoid exclusion of everything */ - if (endtag < chptr) { - tabooPatterns = realloc(tabooPatterns, sizeof(*tabooPatterns) * - (tabooCount + 1)); - bytes = asprintf(&pattern, "*%.*s", (int)(chptr - endtag), endtag); + chptr = endtag; + while (!isspace((unsigned char)*chptr) && *chptr != ',' && *chptr) + chptr++; - /* should test for malloc() failure */ - assert(bytes != -1); - tabooPatterns[tabooCount] = pattern; - tabooCount++; - } + /* accept only non-empty patterns to avoid exclusion of everything */ + if (endtag < chptr) { + tabooPatterns = realloc(tabooPatterns, sizeof(*tabooPatterns) * + (tabooCount + 1)); + bytes = asprintf(&pattern, "*%.*s", (int)(chptr - endtag), endtag); - endtag = chptr; - if (*endtag == ',') - endtag++; - while (*endtag && isspace((unsigned char)*endtag)) - endtag++; + /* should test for malloc() failure */ + assert(bytes != -1); + tabooPatterns[tabooCount] = pattern; + tabooCount++; } + + endtag = chptr; + if (*endtag == ',') + endtag++; + while (*endtag && isspace((unsigned char)*endtag)) + endtag++; } - else continue; } else if (!strcmp(key, "taboopat")) { if (newlog != defConfig) { message(MESS_ERROR, @@ -1389,68 +1384,68 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) continue; } free(key); - if ((key = isolateValue(configFile, lineNum, "taboopat", &start, - &buf, length)) != NULL) { - endtag = key; - if (*endtag == '+') { + key = isolateValue(configFile, lineNum, "taboopat", &start, + &buf, length); + if (key == NULL) + continue; + + endtag = key; + if (*endtag == '+') { + endtag++; + while (isspace((unsigned char)*endtag) && *endtag) endtag++; - while (isspace((unsigned char)*endtag) && *endtag) - endtag++; - } else { - free_2d_array(tabooPatterns, tabooCount); - tabooCount = 0; - /* realloc of NULL is safe by definition */ - tabooPatterns = NULL; - } + } else { + free_2d_array(tabooPatterns, tabooCount); + tabooCount = 0; + /* realloc of NULL is safe by definition */ + tabooPatterns = NULL; + } - while (*endtag) { - int bytes; - char *pattern = NULL; + while (*endtag) { + int bytes; + char *pattern = NULL; - chptr = endtag; - while (!isspace((unsigned char)*chptr) && *chptr != ',' && *chptr) - chptr++; + chptr = endtag; + while (!isspace((unsigned char)*chptr) && *chptr != ',' && *chptr) + chptr++; - tabooPatterns = realloc(tabooPatterns, sizeof(*tabooPatterns) * - (tabooCount + 1)); - bytes = asprintf(&pattern, "%.*s", (int)(chptr - endtag), endtag); + tabooPatterns = realloc(tabooPatterns, sizeof(*tabooPatterns) * + (tabooCount + 1)); + bytes = asprintf(&pattern, "%.*s", (int)(chptr - endtag), endtag); - /* should test for malloc() failure */ - assert(bytes != -1); - tabooPatterns[tabooCount] = pattern; - tabooCount++; + /* should test for malloc() failure */ + assert(bytes != -1); + tabooPatterns[tabooCount] = pattern; + tabooCount++; - endtag = chptr; - if (*endtag == ',') - endtag++; - while (*endtag && isspace((unsigned char)*endtag)) - endtag++; - } + endtag = chptr; + if (*endtag == ',') + endtag++; + while (*endtag && isspace((unsigned char)*endtag)) + endtag++; } - else continue; } else if (!strcmp(key, "include")) { free(key); - if ((key = isolateValue(configFile, lineNum, "include", &start, - &buf, length)) != NULL) { - - message(MESS_DEBUG, "including %s\n", key); - if (recursion_depth >= MAX_NESTING) { - message(MESS_ERROR, "%s:%d include nesting too deep\n", - configFile, lineNum); - logerror = 1; - continue; - } + key = isolateValue(configFile, lineNum, "include", &start, + &buf, length); + if (key == NULL) + continue; + message(MESS_DEBUG, "including %s\n", key); + if (recursion_depth >= MAX_NESTING) { + message(MESS_ERROR, "%s:%d include nesting too deep\n", + configFile, lineNum); + logerror = 1; + continue; + } - ++recursion_depth; - rv = readConfigPath(key, newlog); - --recursion_depth; + ++recursion_depth; + rv = readConfigPath(key, newlog); + --recursion_depth; - if (rv) { - logerror = 1; - continue; - } + if (rv) { + logerror = 1; + continue; } - else continue; } else if (!strcmp(key, "olddir")) { freeLogItem (oldDir); @@ -1460,28 +1455,23 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) } message(MESS_DEBUG, "olddir is now %s\n", newlog->oldDir); } else if (!strcmp(key, "extension")) { - if ((key = isolateValue - (configFile, lineNum, "extension name", &start, - &buf, length)) != NULL) { - freeLogItem (extension); - newlog->extension = key; - key = NULL; - } - else continue; - - message(MESS_DEBUG, "extension is now %s\n", - newlog->extension); + key = isolateValue(configFile, lineNum, "extension name", &start, + &buf, length); + if (key == NULL) + continue; + freeLogItem (extension); + newlog->extension = key; + key = NULL; + message(MESS_DEBUG, "extension is now %s\n", newlog->extension); } else if (!strcmp(key, "addextension")) { - if ((key = isolateValue - (configFile, lineNum, "addextension name", &start, - &buf, length)) != NULL) { - freeLogItem (addextension); - newlog->addextension = key; - key = NULL; - } - else continue; - + key = isolateValue(configFile, lineNum, "addextension name", &start, + &buf, length); + if (key == NULL) + continue; + freeLogItem (addextension); + newlog->addextension = key; + key = NULL; message(MESS_DEBUG, "addextension is now %s\n", newlog->addextension); @@ -1827,7 +1817,8 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) break; case STATE_LOAD_SCRIPT: case STATE_LOAD_SCRIPT | STATE_SKIP_CONFIG: - if ((key = isolateWord(&start, &buf, length)) == NULL) + key = isolateWord(&start, &buf, length); + if (key == NULL) continue; if (strcmp(key, "endscript") == 0) { @@ -1862,7 +1853,8 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) newlog = defConfig; } else { - if ((key = isolateWord(&start, &buf, length)) == NULL) + key = isolateWord(&start, &buf, length); + if (key == NULL) continue; if ( (strcmp(key, "postrotate") == 0) || -- 2.17.1 From a3a955494999bd4861f14b846c345cbc96715262 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Wed, 1 Aug 2018 15:09:40 +0200 Subject: [PATCH 2/3] readConfigFile: assign and free 'key' consistently This commit fixes the following memory leaks (detected by Coverity): Error: RESOURCE_LEAK: config.c:1466: overwrite_var: Overwriting "key" in "key = isolateValue(configFile, lineNum, "extension name", &start, &buf, length)" leaks the storage that "key" points to. Error: RESOURCE_LEAK: config.c:1479: overwrite_var: Overwriting "key" in "key = isolateValue(configFile, lineNum, "addextension name", &start, &buf, length)" leaks the storage that "key" points to. Error: RESOURCE_LEAK: config.c:1043: alloc_fn: Storage is returned from allocation function "isolateWord". config.c:219:2: alloc_fn: Storage is returned from allocation function "strndup". config.c:219:2: assign: Assigning: "key" = "strndup(start, endtag - start)". config.c:221:2: return_alloc: Returning allocated memory "key". config.c:1043: var_assign: Assigning: "key" = storage returned from "isolateWord(&start, &buf, length)". config.c:1928: leaked_storage: Variable "key" going out of scope leaks the storage it points to. Error: RESOURCE_LEAK: config.c:1153: alloc_fn: Storage is returned from allocation function "isolateValue". config.c:204:2: alloc_fn: Storage is returned from allocation function "isolateLine". config.c:178:2: alloc_fn: Storage is returned from allocation function "strndup". config.c:178:2: assign: Assigning: "key" = "strndup(start, endtag - start + 1L)". config.c:180:2: return_alloc: Returning allocated memory "key". config.c:204:2: return_alloc_fn: Directly returning storage allocated by "isolateLine". config.c:1153: var_assign: Assigning: "key" = storage returned from "isolateValue(configFile, lineNum, opt, &start, &buf, length)". config.c:1928: leaked_storage: Variable "key" going out of scope leaks the storage it points to. Error: RESOURCE_LEAK: config.c:1219: alloc_fn: Storage is returned from allocation function "isolateLine". config.c:178:2: alloc_fn: Storage is returned from allocation function "strndup". config.c:178:2: assign: Assigning: "key" = "strndup(start, endtag - start + 1L)". config.c:180:2: return_alloc: Returning allocated memory "key". config.c:1219: var_assign: Assigning: "key" = storage returned from "isolateLine(&start, &buf, length)". config.c:1928: leaked_storage: Variable "key" going out of scope leaks the storage it points to. Closes #208 --- config.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/config.c b/config.c index d2fba10..39c9bc7 100644 --- a/config.c +++ b/config.c @@ -1022,10 +1022,6 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) start = buf; for (start = buf; start - buf < length; start++) { - if (key) { - free(key); - key = NULL; - } switch (state) { case STATE_DEFAULT: if (isblank((unsigned char)*start)) @@ -1037,6 +1033,7 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) } if (isalpha((unsigned char)*start)) { + free(key); key = isolateWord(&start, &buf, length); if (key == NULL) continue; @@ -1455,6 +1452,7 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) } message(MESS_DEBUG, "olddir is now %s\n", newlog->oldDir); } else if (!strcmp(key, "extension")) { + free(key); key = isolateValue(configFile, lineNum, "extension name", &start, &buf, length); if (key == NULL) @@ -1465,6 +1463,7 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) message(MESS_DEBUG, "extension is now %s\n", newlog->extension); } else if (!strcmp(key, "addextension")) { + free(key); key = isolateValue(configFile, lineNum, "addextension name", &start, &buf, length); if (key == NULL) @@ -1557,8 +1556,6 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) if (*start != '\n') state = STATE_SKIP_LINE; } - free(key); - key = NULL; } else if (*start == '/' || *start == '"' || *start == '\'' #ifdef GLOB_TILDE || *start == '~' @@ -1817,6 +1814,7 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) break; case STATE_LOAD_SCRIPT: case STATE_LOAD_SCRIPT | STATE_SKIP_CONFIG: + free(key); key = isolateWord(&start, &buf, length); if (key == NULL) continue; @@ -1853,6 +1851,7 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) newlog = defConfig; } else { + free(key); key = isolateWord(&start, &buf, length); if (key == NULL) continue; @@ -1884,8 +1883,6 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) state = STATE_SKIP_LINE | STATE_SKIP_CONFIG; } } - free(key); - key = NULL; } break; default: @@ -1893,10 +1890,6 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) "%s: %d: readConfigFile() unknown state\n", configFile, lineNum); } - if (key) { - free(key); - key = NULL; - } if (*start == '\n') { lineNum++; } @@ -1910,6 +1903,8 @@ static int readConfigFile(const char *configFile, struct logInfo *defConfig) goto error; } + free(key); + munmap(buf, (size_t) length); close(fd); return logerror; -- 2.17.1 From 771af94fd6c6299a7cb3d20c8b247591775653d3 Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Wed, 1 Aug 2018 16:06:27 +0200 Subject: [PATCH 3/3] simplify code of prerotateSingleLog() ... to eliminate a use-after-free false positive reported by Coverity: Error: USE_AFTER_FREE: logrotate.c:1800: freed_arg: "free" frees "oldName". logrotate.c:1779: use_after_free: Using freed pointer "oldName". Closes #209 --- logrotate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/logrotate.c b/logrotate.c index 02d45e9..95fd70b 100644 --- a/logrotate.c +++ b/logrotate.c @@ -1353,7 +1353,7 @@ static int prerotateSingleLog(struct logInfo *log, int logNum, struct logState *state, struct logNames *rotNames) { struct tm now = *localtime(&nowSecs); - char *oldName, *newName = NULL; + char *oldName = NULL; const char *compext = ""; const char *fileext = ""; int hasErrors = 0; @@ -1670,6 +1670,7 @@ static int prerotateSingleLog(struct logInfo *log, int logNum, free(glob_pattern); } else { int i; + char *newName = NULL; if (log->rotateAge) { struct stat fst_buf; for (i = 1; i <= rotateCount + 1; i++) { @@ -1697,7 +1698,6 @@ static int prerotateSingleLog(struct logInfo *log, int logNum, compext) < 0) { message(MESS_FATAL, "could not allocate disposeName memory\n"); } - newName = strdup(oldName); rotNames->disposeName = strdup(oldName); @@ -1711,6 +1711,8 @@ static int prerotateSingleLog(struct logInfo *log, int logNum, if (asprintf(&oldName, "%s/%s.%d%s%s", rotNames->dirName, rotNames->baseName, i, fileext, compext) < 0) { message(MESS_FATAL, "could not allocate oldName memory\n"); + oldName = NULL; + break; } message(MESS_DEBUG, @@ -1727,11 +1729,9 @@ static int prerotateSingleLog(struct logInfo *log, int logNum, hasErrors = 1; } } - if (hasErrors || i - 1 < 0) - free(oldName); - } free(newName); + free(oldName); } /* !LOG_FLAG_DATEEXT */ if (log->flags & LOG_FLAG_DATEEXT) { -- 2.17.1