The code tested a pointer for NULL after setting it to NULL
a dozen or so lines back. Remove the pointless test and also rearrange
the code a bit to make it clearer.
Details
Details
Run newsyslog test script.
Test output:
1..180
ok 1 - create file
ok 2 - rotate normal 1 cnt=0
ok 3 - rotate normal 2 cnt=0
ok 4 - rotate normal 3 cnt=0
ok 5 - rotate normal 3 cnt=0
ok 6 - noaction
ok 7 - create file
ok 8 - rotate normal 1 cnt=1
ok 9 - rotate normal 2 cnt=1
ok 10 - rotate normal 3 cnt=1
ok 11 - rotate normal 3 cnt=1
ok 12 - noaction
ok 13 - create file
ok 14 - rotate normal 1 cnt=2
ok 15 - rotate normal 2 cnt=2
ok 16 - rotate normal 3 cnt=2
ok 17 - rotate normal 3 cnt=2
ok 18 - noaction
ok 19 - create file
ok 20 - rotate normal 1 cnt=3
ok 21 - rotate normal 2 cnt=3
ok 22 - rotate normal 3 cnt=3
ok 23 - rotate normal 3 cnt=3
ok 24 - noaction
ok 25 - create file .gz
ok 26 - rotate normal 1 cnt=0 .gz
ok 27 - rotate normal 2 cnt=0 .gz
ok 28 - rotate normal 3 cnt=0 .gz
ok 29 - rotate normal 3 cnt=0 .gz
ok 30 - noaction .gz
ok 31 - create file .gz
ok 32 - rotate normal 1 cnt=1 .gz
ok 33 - rotate normal 2 cnt=1 .gz
ok 34 - rotate normal 3 cnt=1 .gz
ok 35 - rotate normal 3 cnt=1 .gz
ok 36 - noaction .gz
ok 37 - create file .gz
ok 38 - rotate normal 1 cnt=2 .gz
ok 39 - rotate normal 2 cnt=2 .gz
ok 40 - rotate normal 3 cnt=2 .gz
ok 41 - rotate normal 3 cnt=2 .gz
ok 42 - noaction .gz
ok 43 - create file .gz
ok 44 - rotate normal 1 cnt=3 .gz
ok 45 - rotate normal 2 cnt=3 .gz
ok 46 - rotate normal 3 cnt=3 .gz
ok 47 - rotate normal 3 cnt=3 .gz
ok 48 - noaction .gz
ok 49 - create file
ok 50 - rotate normal 1
ok 51 - rotate normal 2
ok 52 - rotate normal 3
ok 53 - rotate normal 4
ok 54 - rotate normal 5
ok 55 - noaction
ok 56 - create file .gz
ok 57 - rotate normal 1 .gz
ok 58 - rotate normal 2 .gz
ok 59 - rotate normal 3 .gz
ok 60 - rotate normal 4 .gz
ok 61 - rotate normal 5 .gz
ok 62 - noaction .gz
ok 63 - create file .bz2
ok 64 - rotate normal 1 .bz2
ok 65 - rotate normal 2 .bz2
ok 66 - rotate normal 3 .bz2
ok 67 - rotate normal 4 .bz2
ok 68 - rotate normal 5 .bz2
ok 69 - noaction .bz2
ok 70 - create file .xz
ok 71 - rotate normal 1 .xz
ok 72 - rotate normal 2 .xz
ok 73 - rotate normal 3 .xz
ok 74 - rotate normal 4 .xz
ok 75 - rotate normal 5 .xz
ok 76 - noaction .xz
ok 77 - create file .zst
ok 78 - rotate normal 1 .zst
ok 79 - rotate normal 2 .zst
ok 80 - rotate normal 3 .zst
ok 81 - rotate normal 4 .zst
ok 82 - rotate normal 5 .zst
ok 83 - noaction .zst
ok 84 - create file archive dir
ok 85 - rotate normal 1 archive dir
ok 86 - rotate normal 2 archive dir
ok 87 - rotate normal 3 archive dir
ok 88 - rotate normal 4 archive dir
ok 89 - rotate normal 5 archive dir
ok 90 - noaction archive dir
ok 91 - create file .gz archive dir
ok 92 - rotate normal 1 .gz archive dir
ok 93 - rotate normal 2 .gz archive dir
ok 94 - rotate normal 3 .gz archive dir
ok 95 - rotate normal 4 .gz archive dir
ok 96 - rotate normal 5 .gz archive dir
ok 97 - noaction .gz archive dir
ok 98 - create file .bz2 archive dir
ok 99 - rotate normal 1 .bz2 archive dir
ok 100 - rotate normal 2 .bz2 archive dir
ok 101 - rotate normal 3 .bz2 archive dir
ok 102 - rotate normal 4 .bz2 archive dir
ok 103 - rotate normal 5 .bz2 archive dir
ok 104 - noaction .bz2 archive dir
ok 105 - create file .xz archive dir
ok 106 - rotate normal 1 .xz archive dir
ok 107 - rotate normal 2 .xz archive dir
ok 108 - rotate normal 3 .xz archive dir
ok 109 - rotate normal 4 .xz archive dir
ok 110 - rotate normal 5 .xz archive dir
ok 111 - noaction .xz archive dir
ok 112 - create file .zst archive dir
ok 113 - rotate normal 1 .zst archive dir
ok 114 - rotate normal 2 .zst archive dir
ok 115 - rotate normal 3 .zst archive dir
ok 116 - rotate normal 4 .zst archive dir
ok 117 - rotate normal 5 .zst archive dir
ok 118 - noaction .zst archive dir
ok 119 - create file
ok 120 - rotate time 1
ok 121 - rotate time 2
ok 122 - rotate time 3
ok 123 - rotate time 4
ok 124 - noaction
ok 125 - create file gz
ok 126 - rotate time 1 gz
ok 127 - rotate time 2 gz
ok 128 - rotate time 3 gz
ok 129 - rotate time 4 gz
ok 130 - noaction gz
ok 131 - create file bz2
ok 132 - rotate time 1 bz2
ok 133 - rotate time 2 bz2
ok 134 - rotate time 3 bz2
ok 135 - rotate time 4 bz2
ok 136 - noaction bz2
ok 137 - create file xz
ok 138 - rotate time 1 xz
ok 139 - rotate time 2 xz
ok 140 - rotate time 3 xz
ok 141 - rotate time 4 xz
ok 142 - noaction xz
ok 143 - create file zst
ok 144 - rotate time 1 zst
ok 145 - rotate time 2 zst
ok 146 - rotate time 3 zst
ok 147 - rotate time 4 zst
ok 148 - noaction zst
ok 149 - create file archive dir
ok 150 - rotate time 1 archive dir
ok 151 - rotate time 2 archive dir
ok 152 - rotate time 3 archive dir
ok 153 - rotate time 4 archive dir
ok 154 - noaction archive dir
ok 155 - create file gz archive dir
ok 156 - rotate time 1 gz archive dir
ok 157 - rotate time 2 gz archive dir
ok 158 - rotate time 3 gz archive dir
ok 159 - rotate time 4 gz archive dir
ok 160 - noaction gz archive dir
ok 161 - create file bz2 archive dir
ok 162 - rotate time 1 bz2 archive dir
ok 163 - rotate time 2 bz2 archive dir
ok 164 - rotate time 3 bz2 archive dir
ok 165 - rotate time 4 bz2 archive dir
ok 166 - noaction bz2 archive dir
ok 167 - create file xz archive dir
ok 168 - rotate time 1 xz archive dir
ok 169 - rotate time 2 xz archive dir
ok 170 - rotate time 3 xz archive dir
ok 171 - rotate time 4 xz archive dir
ok 172 - noaction xz archive dir
ok 173 - create file zst archive dir
ok 174 - rotate time 1 zst archive dir
ok 175 - rotate time 2 zst archive dir
ok 176 - rotate time 3 zst archive dir
ok 177 - rotate time 4 zst archive dir
ok 178 - noaction zst archive dir
ok 179 - RFC-5424 - create file
ok 180 - RFC-5424 - rotate normal 1
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| usr.sbin/newsyslog/newsyslog.c | ||
|---|---|---|
| 843 ↗ | (On Diff #53653) | Changed here to explicitly pass the NULL that was implicitly specified before by assigning NULL to defconf and then passing it. No actual behavior change. |
| 859 ↗ | (On Diff #53653) | This was the dead code identified by Coverity. Because it was dead, measuring code coverage would show this as always uncovered. |
| 871 ↗ | (On Diff #53653) | This test is always true, so there is no point in making it. |
Comment Actions
My guess is that the author intended for defconf input to parse_file() to be a *reference* rather than a bare pointer. If you look at parse_file(), you can see it initializes its copy of defconf_p when the special entry "<default>" is encountered in a file. However, that pointer is never stored anywhere and is leaked when parse_file() returns.
I would propose something like this instead:
--- a/usr.sbin/newsyslog/newsyslog.c +++ b/usr.sbin/newsyslog/newsyslog.c @@ -253,7 +253,7 @@ static const char *path_syslogpid = _PATH_SYSLOGPID; static struct cflist *get_worklist(char **files); static void parse_file(FILE *cf, struct cflist *work_p, struct cflist *glob_p, - struct conf_entry *defconf_p, struct ilist *inclist); + struct conf_entry **defconf, struct ilist *inclist); static void add_to_queue(const char *fname, struct ilist *inclist); static char *sob(char *p); static char *son(char *p); @@ -841,7 +841,7 @@ get_worklist(char **files) if (verbose) printf("Processing %s\n", inc->file); - parse_file(f, filelist, globlist, defconf, &inclist); + parse_file(f, filelist, globlist, &defconf, &inclist); (void) fclose(f); } @@ -1046,7 +1046,7 @@ expand_globs(struct cflist *work_p, struct cflist *glob_p) */ static void parse_file(FILE *cf, struct cflist *work_p, struct cflist *glob_p, - struct conf_entry *defconf_p, struct ilist *inclist) + struct conf_entry **defconf_p, struct ilist *inclist) { char line[BUFSIZ], *parse, *q; char *cp, *errline, *group; @@ -1137,12 +1137,12 @@ parse_file(FILE *cf, struct cflist *work_p, struct cflist *glob_p, working = init_entry(q, NULL); if (strcasecmp(DEFAULT_MARKER, q) == 0) { special = 1; - if (defconf_p != NULL) { + if (*defconf_p != NULL) { warnx("Ignoring duplicate entry for %s!", q); free_entry(working); continue; } - defconf_p = working; + *defconf_p = working; } q = parse = missing_field(sob(parse + 1), errline);
This regression was introduced in r208648 (2010), so maybe it isn't too important.
Comment Actions
That would make sense, and also fix another CID. I was originally going for the "don't change behavior" approach, but since you already verified the regression, I think it makes sense to go along with your suggestion, even though newsyslog has apparently been getting along just fine without the old functionality for 9 years.
Comment Actions
Taking @cem's suggestion for an alternative fix. While I was at it, I
expanded the scope to pick up some fixes for additional CIDs:
- CID 1394815, CID 1305673: Dereference before null check - memory was allocated and the allocation checked for NULL with a call to errx() if it failed. Code below that was guaranteed that the pointer was non-NULL, but there was another check for NULL at the exit of the function (after the memory had already been referenced). Eliminate the useless NULL check.
- CID 1007454, CID 1007453: Resource leak - The result of a strdup() was stored in a global variable and not freed before program exit.
- CID 1007452: Resource leak - Storage intended to be allocated and returned to the caller was never freed. This was the result of a regression in the function signature introduced in r208648 (2010) (thanks for that find, @cem!). Fixed by altering the function signature and passing the allocated memory to the caller as intended.
- CID 1008620: Logically dead code in newsyslog.c - This was a direct result of CID 1007452. Since the memory allocated as described there was not returned to the caller, a subsequent check for the memory having been allocated was dead code. Returning the memory re-animates the code that is the subject of this CID.
- CID 1006131: Unused value - in parsing a configuration file, a pointer to the end of the last field was saved, but not used after that. Rewrite to use the pointer value. This could have been fixed by avoiding the assignment altogether, but this solutions more closely follows the pattern used in the preceding code.
Comment Actions
Reverted the change and am re-committing without these changes:
CID 1007454, CID 1007453: Resource leak - The result of a strdup() was stored in a global variable and not freed before program exit.
As @cem pointed out, the memory is going to get freed anyway and there are other code paths that will still "leak" the memory. After I'd committed the changes there was additional discussion on the mailing list that convinced me it was a bad idea to sprinkle the free() calls around that would deallocate this memory.
Comment Actions
Yeah, sorry about that bikeshed. IMO the two lines don't matter, either way. The FreeBSD community leans towards exit-is-obviously-cleanup-for-C-programs, while ISLN leans toward "appease Coverity's false positive memory leaks at all costs." So you may well have to carry the patch internally anyway. The latter is good practice if you ever want to convert main() to a library function; however, I've literally never run into this problem in ~14 years of using C, so I'm not sure it's a great argument.
If you have time, I'd encourage some investigation into how we can configure Coverity (both FreeBSD's public Coverity Scan instance, and our corporate instance) to ignore this kind of report entirely (for C programs). Maybe there is a setting somewhere. Maybe we can somehow do this with modeling (I am not sure how that would work, though).
(Going on a tangent) I suspect the reason Coverity bothers to detect this kind of thing is for C++ static analysis, not C. C just gets grandfathered in.
C programs do not have this problem. I think it may be safe to ignore destruction of plain-old-data C++ class-objects in C++ as well, but (a) I don't proclaim any C++ proficiency and may be wrong, and (b) I'm even less hopeful Coverity can detect that particular condition (and ignore it) than detecting C compilation units. I would love to be surprised :-).