Bug 24798

On Fri, Feb 02, 2001 at 01:37:06PM +0000, nick@netability.ie wrote:
> 
> >Number:         24798
> >Category:       bin
> >Synopsis:       pac dumps core if printer accounting not enabled
> >Responsible:    freebsd-bugs
> >Originator:     Nick Hilliard
> >Release:        FreeBSD 4.2-RELEASE i386
> >Organization:
> Network Ability Ltd.
> >Environment:
> 
> 	/etc/printcap contains:
> 
> lp|printer:\
>         :sh:\
>         :rm=printer:sd=/var/spool/output/jimbo:lf=/var/log/lpd-errs:
> 
> >Description:
> 
> 	When pac runs, it assumes that the default or specified printer has
> 	accounting enabled.  If this is not the case, then pp->acct_file on
> 	line 453 of pac.c will be set to NULL.  A couple of lines further
> 	down, acctfile (=pp->acct_file) is dereferenced, which causes a SEGV.
> 
> >How-To-Repeat:
> 
> 	put the entry above into /etc/printcap and then type "pac".  A
> 	segmentation fault will occur.
> 
> >Fix:
> 
> 	Here's a patch to fix the problem.  As pac only processes one
> 	printer at a time, it's ok to exit instead of returning an error to
> 	the calling function.
> 
> --- pac.c.orig	Fri Feb  2 13:27:19 2001
> +++ pac.c	Fri Feb  2 13:19:20 2001
> @@ -450,7 +450,10 @@
>  	case PCAPERR_TCLOOP:
>  		fatal(pp, "%s", pcaperr(stat));
>  	}
> -	acctfile = pp->acct_file;
> +	if ((acctfile = pp->acct_file) == NULL) {
> +		printf("pac: accounting not enabled for %s\n", s);
> +		exit(3);
> +	}
>  	if (!pflag && pp->price100)
>  		price = pp->price100/10000.0;
>  	sumfile = (char *) calloc(sizeof(char), strlen(acctfile)+5);

Good catch!

This printf + exit could be rewritten in a more BSD-ish style as:

	err(3, "accounting not enabled for %s", s);

Since src/usr.sbin/pac.c already includes err.h, nothing more is needed.

Hmm..  is Garance A. Drosehn now our official lpr'n'friends maintainer?
Does he get to tackle this PR? :)

G'luck,
Peter

-- 
I am jealous of the first word in this sentence.

> This printf + exit could be rewritten in a more BSD-ish style as:
> 
> 	err(3, "accounting not enabled for %s", s);

yeah, probably.  Actually, I was just imitating the printf() & exit() a few
lines before for consistency's sake.  For convenience, here's the same thing
using err:

--- pac.c.orig	Wed Feb 21 10:53:33 2001
+++ pac.c	Wed Feb 21 10:55:50 2001
@@ -450,7 +450,8 @@
 	case PCAPERR_TCLOOP:
 		fatal(pp, "%s", pcaperr(stat));
 	}
-	acctfile = pp->acct_file;
+	if ((acctfile = pp->acct_file) == NULL)
+		err(3, "accounting not enabled for %s", s);
 	if (!pflag && pp->price100)
 		price = pp->price100/10000.0;
 	sumfile = (char *) calloc(sizeof(char), strlen(acctfile)+5);

Anyway, now that there's a definite schedule for 4.3-release, can someone
organise to get one or other of these patchlets committed before the code
freeze?

Nick

Comment 3 dwmalone 2001-02-25 13:51:42 UTC

State Changed
From-To: open->closed

Patch committed to -current, I'll merge it in a few days all going well.