Subject: bin/7592: programs' error handle broken by good intentions
To: None <gnats-bugs@gnats.netbsd.org>
From: None <cgd@netbsd.org>
List: netbsd-bugs
Date: 05/16/1999 23:20:58
>Number: 7592
>Category: bin
>Synopsis: -Wformat err() 'fixes' broke lots of programs.
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: bin-bug-people (Utility Bug People)
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sun May 16 23:20:00 1999
>Last-Modified:
>Originator: Chris G. Demetriou
>Organization:
Chris Demetriou - cgd@netbsd.org - http://www.netbsd.org/People/Pages/cgd.html
Disclaimer: Not speaking for NetBSD, just expressing my own opinion.
>Release: NetBSD 1.3.x, 1.4, -current
>Environment:
System: NetBSD 1.3.2+ (SPEEDY) #17: Sun Nov 29 16:32:25 PST 1998 cgd@xxx:/a/users/cgd/proj/netbsd/src-1-3-branch/sys/arch/i386/compile/SPEEDY
And inspection of 1.4 and -current sources.
>Description:
[ I'd noticed this brokenness a ... long time ago, but only recently
had time to look into it. to be quite honest, i'm fairly shocked
and dismayed. ]
Many programs' error reporting behaviour is broken. For instance,
when mount_cd9660's mount() request fails, it spits out messages like:
68 [speedy] log % mount -t cd9660 foo bar
mount_cd9660: : No such file or directory
note the extra " :".
This is cause by incorrect use of err(), in which it is invoked
like:
err(1, "%s", "");
This code is broken. To do what it is trying to do, the correct
invocation of err() is:
err(1, NULL);
However, in order to 'fix' gcc -Wformat 'problems,' certain people
took it upon themselves to break the code, either by writing
broken code to begin with or, worse, converting from the correct
call form to the incorrect form to silence warnings!
As of a mid-April -current (trunk) source tree, there were
52 instances of broken calls to err() in 37 files. They were
found with a:
find . -name "*.[ch]" | xargs grep 'err(1, "%s", "")'
over the source tree. (Now that I think about it, there may be
problems w/ calls to err() with other exit codes.)
Using 'cvs annotate' shows that the lines containing the bogus
calls were touched by the following people with the following
frequencies:
28 christos
14 lukem
6 mrg
1 wsanchez, mark, kleink, fair
(I would guess that the first three folks made a lot of 'fixes',
and that the rest saw the bad example and copied it in new code,
but I cannot be sure.)
>How-To-Repeat:
Run commands or examine sources and function documentation
as described in the Description section.
>Fix:
(1) revert all calls of err() like:
err(1, "%s", "")
to:
err(1, NULL)
(2) If printf-like format checking is desired on {err,warn}{,x}()
calls, make gcc aware that in some circumstances NULL as the format
argument is OK, and indicate that it's OK for {err,warn}{,x}().
(3) Don't let developers who do not undersand the notion that warning
fixes MUST NOT BREAK CODE AND MUST BE CHECKED CAREFULLY AND TESTED
make unsupervised warning-related 'fixes' to the source tree. (This
type of thing has happened _way_ too many times, and has wasted too
much time and effort to be tolerated much longer.)
Note that changing err() (or the other functions to tolerate
this brokenness is absolutely wrong.
>Audit-Trail:
>Unformatted: