Bug 166510 - Logrotate should rotate logs even if there are some errors on the configuration
Logrotate should rotate logs even if there are some errors on the configuration
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: logrotate (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Vrabec
:
Depends On:
Blocks: 218388
  Show dependency treegraph
 
Reported: 2005-08-22 13:55 EDT by Filipe Brandenburger
Modified: 2009-07-07 14:39 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-23 07:18:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
A patch fixing few issues with config file parsing (790 bytes, patch)
2006-08-25 11:11 EDT, Jan Cholasta
no flags Details | Diff
A patch fixing few issues with config file parsing (7.99 KB, patch)
2006-09-01 06:22 EDT, Jan Cholasta
no flags Details | Diff
A patch fixing few issues with config file parsing (8.47 KB, patch)
2006-09-01 07:09 EDT, Jan Cholasta
no flags Details | Diff
A new version of the patch (10.90 KB, patch)
2006-09-04 09:15 EDT, Jan Cholasta
no flags Details | Diff
New version of the patch (10.57 KB, patch)
2006-09-05 09:37 EDT, Jan Cholasta
no flags Details | Diff
another fix candidate (10.98 KB, patch)
2006-09-07 07:46 EDT, Peter Vrabec
no flags Details | Diff
Another new version of the patch (11.46 KB, patch)
2006-09-07 11:05 EDT, Jan Cholasta
no flags Details | Diff
New version of the patch (16.30 KB, patch)
2006-09-23 05:07 EDT, Jan Cholasta
no flags Details | Diff

  None (edit)
Description Filipe Brandenburger 2005-08-22 13:55:42 EDT
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050720 Fedora/1.0.6-1.1.fc3 Firefox/1.0.6

Description of problem:
Logrotate should continue rotating logs even if there are some non-fatal errors on the configuration files.

The most common reason I have problems with is: "log file was listed more than once in the configuration". In this specific case, I guess it should apply the first rule it finds in the configuration, ignoring the other one. Frankly, it could implement whatever, the first, the last, the most restrictive, the less restrictive, apply all of them one after the other, or even not apply any rotation for *THIS* log only!!! (this last one I dislike a bit, but it's better than what logrotate does now).

What I think it's inadmissable is that logrotate stops rotating *ALL* logs because of some error in configuration of one of them.

You may say the problem is with me, and that I did the wrong configuration, but it happens a lot that I configure the rotating for some log in /etc/logrotate.conf, and then I ask the developpers to include logrotation in a file in /etc/logrotate.d/ in the RPM package. When I upgrade the RPM, if I forget changing /etc/logrotate.conf again (sometimes another guy did it, so I'm not always sure about that), and that's it, logrotate stops working and my hosts fill their disks with logs!!! It's not always obvious that I have to check logrotate, after all, all I did was install an RPM (it could even be automated install with yum).


Version-Release number of selected component (if applicable):
logrotate-3.7.1-2

How reproducible:
Always

Steps to Reproduce:
1. cp /etc/logrotate.d/syslog /etc/logrotate.d/stopworking
2. /usr/sbin/logrotate /etc/logrotate.conf


Actual Results:  # /usr/sbin/logrotate /etc/logrotate.conf
error: syslog:1 duplicate log entry for /var/log/messages


Expected Results:  Logrotate should issue warnings about duplicate log entries, but should rotate the logs anyway.

Additional info:
Comment 1 Jan Cholasta 2006-08-25 11:11:15 EDT
Created attachment 134926 [details]
A patch fixing few issues with config file parsing

The patch fixes this bug, #185538 and #182062. It's very simple - if logrotate
finds a duplicate filename (or corrup config file), it skips it and continues
with the next one.
Comment 2 Peter Vrabec 2006-08-28 08:16:53 EDT
Thnx. for patch, but it doesn't fix all cases. If there is a bug in local scope of configuration file, 
log file won't be skipped.

example:
$ cat rot/foo2
create
missingok
/tmp/foo2 {
        rotate -3
}

$sudo logrotate-3.7.4/logrotate -vf rotFile
...
reading config info for /tmp/foo2
error: foo2:4 bad rotation count '-3'
error: found error in foo2, skipping
reading config file foo3
....
rotating pattern: /tmp/foo2  forced from command line (-3 rotations)
empty log files are rotated, old logs are removed
considering log /tmp/foo2
  log needs rotating
rotating log /tmp/foo2, log->rotateCount is -3
log /tmp/foo2.-2 doesn't exist -- won't try to dispose of it
renaming /tmp/foo2 to /tmp/foo2.1
creating new log mode = 0664 uid = 500 gid = 500
...
Comment 3 Jan Cholasta 2006-09-01 06:22:38 EDT
Created attachment 135366 [details]
A patch fixing few issues with config file parsing

New version of the patch.
Comment 4 Jan Cholasta 2006-09-01 07:06:13 EDT
Comment on attachment 135366 [details]
A patch fixing few issues with config file parsing

>--- logrotate-3.7.4/config.c.old	2006-07-24 14:08:04.000000000 +0200
>+++ logrotate-3.7.4/config.c	2006-09-01 11:56:52.000000000 +0200
>@@ -267,10 +267,8 @@
> 
> 	    if (readConfigFile(namelist[i], defConfig, logsPtr,
> 			       numLogsPtr)) {
>-		fchdir(here);
>-		close(here);
>-		free_2d_array(namelist, files_count);
>-		return 1;
>+		message(MESS_ERROR, "found error in %s, skipping\n", namelist[i]);
>+		continue;
> 	    }
> 	}
> 
>@@ -364,6 +362,7 @@
>     glob_t globResult;
>     const char **argv;
>     int argc, argNum;
>+	int logerror = 0;
> 
>     /* FIXME: createOwner and createGroup probably shouldn't be fixed
>        length arrays -- of course, if we aren't run setuid it doesn't
>@@ -417,6 +416,34 @@
> 
>     start = buf;
>     while (*start) {
>+	if (logerror) {
>+		while (*start != '}') {
>+			if (*start == 0) {
>+				message(MESS_ERROR, "%s:%d } expected \n", configFile, lineNum);
>+				return 1;
>+			}
>+			else if (*start == '\n') {
>+				lineNum++;
>+			}
>+			start++;
>+		}
>+
>+		if (newlog->files)
>+			free(newlog->files);
>+		if (newlog->pre && newlog->pre != defConfig->pre)
>+			free(newlog->pre);
>+		if (newlog->post && newlog->post != defConfig->post)
>+			free(newlog->post);
>+		if (newlog->first && newlog->first != defConfig->first)
>+			free(newlog->first);
>+		if (newlog->last && newlog->last != defConfig->last)
>+			free(newlog->last);
>+		(*numLogsPtr)--;
>+	    *logsPtr = realloc(*logsPtr, sizeof(**logsPtr) * *numLogsPtr);
>+		newlog = defConfig;
>+
>+		logerror = 0;
>+	}
> 	while (isblank(*start) && (*start))
> 	    start++;
> 	if (*start == '#') {
>@@ -549,7 +576,12 @@
> 		if (rc == 4) {
> 		    message(MESS_ERROR, "%s:%d extra arguments for "
> 			    "create\n", configFile, lineNum);
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		if (rc > 0)
>@@ -560,7 +592,12 @@
> 		    if (!pw) {
> 			message(MESS_ERROR, "%s:%d unknown user '%s'\n",
> 				configFile, lineNum, createOwner);
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		    }
> 		    newlog->createUid = pw->pw_uid;
> 		    endpwent();
>@@ -570,7 +607,12 @@
> 		    if (!group) {
> 			message(MESS_ERROR, "%s:%d unknown group '%s'\n",
> 				configFile, lineNum, createGroup);
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		    }
> 		    newlog->createGid = group->gr_gid;
> 		    endgrent();
>@@ -605,7 +647,12 @@
> 		    } else if (!isdigit(start[length])) {
> 			message(MESS_ERROR, "%s:%d unknown unit '%c'\n",
> 				configFile, lineNum, start[length]);
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		    } else {
> 			multiplier = 1;
> 		    }
>@@ -614,7 +661,12 @@
> 		    if (*chptr) {
> 			message(MESS_ERROR, "%s:%d bad size '%s'\n",
> 				configFile, lineNum, start);
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		    }
> 
> 		    if (!strcmp(opt, "size")) {
>@@ -676,7 +728,12 @@
> 			message(MESS_ERROR,
> 				"%s:%d bad rotation count '%s'\n",
> 				configFile, lineNum, start);
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		    }
> 		    *endtag = oldchar, start = endtag;
> 		}
>@@ -692,7 +749,12 @@
> 		    if (*chptr || newlog->logStart < 0) {
> 			message(MESS_ERROR, "%s:%d bad start count '%s'\n",
> 				configFile, lineNum, start);
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		    }
> 		    *endtag = oldchar, start = endtag;
> 		}
>@@ -708,7 +770,12 @@
> 		    if (*chptr || newlog->rotateAge < 0) {
> 			message(MESS_ERROR, "%s:%d bad maximum age '%s'\n",
> 				configFile, lineNum, start);
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		    }
> 		    *endtag = oldchar, start = endtag;
> 		}
>@@ -720,7 +787,12 @@
> 		*endtag = oldchar, start = endtag;
> 		if (!(newlog->logAddress = readAddress(configFile, lineNum,
> 						       "mail", &start))) {
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 	    } else if (!strcmp(start, "nomail")) {
> 		/* hmmm, we could lose memory here, but not much */
>@@ -773,7 +845,12 @@
> 			    "%s:%d tabooext may not appear inside "
> 			    "of log file definition\n", configFile,
> 			    lineNum);
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		*endtag = oldchar, start = endtag;
>@@ -819,7 +896,12 @@
> 			    "%s:%d include may not appear inside "
> 			    "of log file definition\n", configFile,
> 			    lineNum);
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		*endtag = oldchar, start = endtag;
>@@ -831,7 +913,12 @@
> 
> 		    if (readConfigPath(start, defConfig, logsPtr,
> 				       numLogsPtr))
>-			return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 
> 		    *endtag = oldchar, start = endtag;
> 		}
>@@ -839,7 +926,12 @@
> 		*endtag = oldchar, start = endtag;
> 		if (!(newlog->oldDir = readPath(configFile, lineNum,
> 						"olddir", &start))) {
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> #if 0
> 		if (stat(newlog->oldDir, &sb)) {
>@@ -881,7 +973,12 @@
> 		if (!
> 		    (newlog->compress_prog =
> 		     readPath(configFile, lineNum, "compress", &start))) {
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		if (access(newlog->compress_prog, X_OK)) {
>@@ -889,7 +986,12 @@
> 			    "%s:%d compression program %s is not an executable file\n",
> 			    configFile, lineNum, newlog->compress_prog);
> 		    free(newlog->compress_prog);
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		message(MESS_DEBUG, "compress_prog is now %s\n",
>@@ -901,7 +1003,12 @@
> 		    (newlog->uncompress_prog =
> 		     readPath(configFile, lineNum, "uncompress",
> 			      &start))) {
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		if (access(newlog->uncompress_prog, X_OK)) {
>@@ -909,7 +1016,12 @@
> 			    "%s:%d uncompression program %s is not an executable file\n",
> 			    configFile, lineNum, newlog->uncompress_prog);
> 		    free(newlog->uncompress_prog);
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		message(MESS_DEBUG, "uncompress_prog is now %s\n",
>@@ -923,7 +1035,12 @@
> 		    (options =
> 		     readPath(configFile, lineNum, "compressoptions",
> 			      &start))) {
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		if (poptParseArgvString(options,
>@@ -933,7 +1050,12 @@
> 			    "%s:%d invalid compression options\n",
> 			    configFile, lineNum);
> 		    free(options);
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		message(MESS_DEBUG, "compress_options is now %s\n",
>@@ -945,7 +1067,12 @@
> 		    (newlog->compress_ext =
> 		     readPath(configFile, lineNum, "compress-ext",
> 			      &start))) {
>-		    return 1;
>+			if (newlog != defConfig) {
>+				logerror = 1;
>+				continue;
>+			} else {
>+			    return 1;
>+			}
> 		}
> 
> 		message(MESS_DEBUG, "compress_ext is now %s\n",
>@@ -973,7 +1100,8 @@
> 	    if (newlog != defConfig) {
> 		message(MESS_ERROR, "%s:%d unexpected log filename\n",
> 			configFile, lineNum);
>-		return 1;
>+		logerror = 1;
>+		continue;
> 	    }
> 
> 	    /* If no compression options were found in config file,
>@@ -1042,11 +1170,10 @@
> 			    if (!strcmp((*logsPtr)[j].files[k],
> 					globResult.gl_pathv[i])) {
> 				message(MESS_ERROR,
>-					"%s:%d duplicate log entry for %s\n",
>+					"%s:%d duplicate log entry for %s, skipping\n",
> 					configFile, lineNum,
> 					globResult.gl_pathv[i]);
>-				globfree(&globResult);
>-				return 1;
>+				continue;
> 			    }
> 			}
> 		    }
>@@ -1067,7 +1194,7 @@
> 	    start = endtag + 1;
> 	} else if (*start == '}') {
> 	    if (newlog == defConfig) {
>-		message(MESS_ERROR, "%s:%d unxpected }\n", configFile,
>+		message(MESS_ERROR, "%s:%d unexpected }\n", configFile,
> 			lineNum);
> 		return 1;
> 	    }
Comment 5 Jan Cholasta 2006-09-01 07:09:25 EDT
Created attachment 135370 [details]
A patch fixing few issues with config file parsing

Sorry about the above post (my bugzilla skill is not good enough).
Comment 6 Jan Cholasta 2006-09-04 09:15:50 EDT
Created attachment 135492 [details]
A new version of the patch

The patch fixes these issues:
a) When a config file is corrupt, logrotate skips it and rolls back all
changes.
b) When a filename in log config is duplicate, logrotate skips it.
c) When a log config is corrupt, logrotate skips it.
Comment 7 Jan Cholasta 2006-09-05 09:37:34 EDT
Created attachment 135558 [details]
New version of the patch

New fix - now when logrotate finds duplicate filename, it skips the whole log
config.
Comment 8 Peter Vrabec 2006-09-07 07:46:52 EDT
Created attachment 135738 [details]
another fix candidate

This is adjusted grubber's patch, it was necessary to fix memory leak .
Comment 9 Jan Cholasta 2006-09-07 11:05:53 EDT
Created attachment 135775 [details]
Another new version of the patch

More fixes, including Peter's changes.
Comment 10 Jan Cholasta 2006-09-23 05:07:35 EDT
Created attachment 136992 [details]
New version of the patch

Another version of the patch, now without memory leaks.
Comment 11 Peter Vrabec 2007-01-23 07:18:29 EST
thnx. to Jan Cholasta

fixed in:
FC6(testing) logrotate-3.7.4-10.fc6,
rawhide logrotate-3.7.4-11.fc7.

Note You need to log in before you can comment on or make changes to this bug.