bin/139470: pkg_delete segfaults on '@pkgdep ' in +CONTENTS file

[TerryP]

>Number:         139470
>Category:       bin
>Synopsis:       pkg_delete segfaults on '@pkgdep ' in +CONTENTS file
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sat Oct 10 04:50:06 UTC 2009
>Closed-Date:
>Last-Modified:
>Originator:     TerryP
>Release:        7-stable / RELENG_7
>Organization:
N/A
>Environment:
FreeBSD dixie.launchmodem.com 7.2-STABLE FreeBSD 7.2-STABLE #4: Thu Jun 25 22:05:05 UTC 2009     root at dixie.launchmodem.com:/usr/obj/usr/src/sys/VIPER  i386

>Description:
While running make deinstall in /usr/ports/graphics/ImageMagick, pkg_delete crashes with the following output:

pkg_delete: package 'ImageMagick-6.5.5.7' is required by these other packages
and may not be deinstalled (but I'll delete it anyway):
inkscape-0.46_6
libxine-1.1.16.3_1
Segmentation fault


I've read the source code in /usr/src/usr.sbin/pkg_install and found out what happened. The list that is processed contains a NULL entry, which is later passed onto strcmp() as part of the processing; you can get the exact backtrace through gdb. It looks like the origin of the NULL in question, is found within the function read_plist() from pkg_install/lib/plist.c. In this function, the lines are read from the currently installed /var/db/pkg/pkgname/+CONTENTS file. If the last character in the parsed line, is a space then it is replaced with a \0 terminator, until trailing spaces are blotted out. Within the +CONTENTS file for the installed ImageMagick here, is a line like this: '@pkgdep ', note the trailing space.

Parsing that line results in this code being executed as part of read_plist():

        if (*cp == '\0') {
            cp = NULL;
            goto bottom;
        }

the NULL gets passed on, until it eventually becomes an argument to strcmp(), which obviously is a cork poping event!


I am not familiar with the syntax of directives used in the +CONTENTS files, but I know that pkg_delete should never crash ;). There is probably a problem with the ImageMagick port or the code that generates the packing list in +CONTENTS.
>How-To-Repeat:
I believe all you would have to do, is run pkg_delete -f pkgname, where pkgname has a /var/db/pkg/pkgname/+CONTENTS file with a bad directive comparable to this one: '@pkgdep ', again note the trailing space and lack of an argument to the pkgdep command.

This was found while trying to deinstall ImageMagick-6.5.5.7.
>Fix:
0.) Decide how read_plist() should deal with parsing errors
1.) Generate warning about +CONTENTS having a bad line.
2.) Do anything except pass on a NULL pointer further down the chain.

My advice is that the bad directive should generate a warning and be ignored, or the whole thing should just exit with an error message.


Attached is a unified diff/patch file that makes it exit with an error message instead of segfault. Someone who works on FreeBSD is a much better judge of what behaviour is actually desired here, so I will leave the wisdom department to y'all.

cd /usr/src/usr.sbin/pkg_install
patch < /path/to/patchfile

Patch attached with submission follows:

--- lib/plist.c	2009-10-10 04:20:46.000000000 +0000
+++ lib/plist.c.changed	2009-10-10 04:28:50.000000000 +0000
@@ -284,8 +284,8 @@
 	    goto bottom;
 	}
 	if (*cp == '\0') {
-	    cp = NULL;
-	    goto bottom;
+	    errx(2, "%s: error in line '%s', the +CONTENTS file for %s is probably bad",
+	          __func__, pline, pkg->name);
 	}
 	if (cmd == PLIST_COMMENT && sscanf(cp, "PKG_FORMAT_REVISION:%d.%d\n",
 					   &major, &minor) == 2) {


>Release-Note:
>Audit-Trail:
>Unformatted: