From a4ac21e9a8cfe8a73471a195308a742e07d7fe8d Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
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 <kdudka@redhat.com>
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 <kdudka@redhat.com>
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