pkg/38230: pkgtools/pkg_comp leaves turds in ${SANDBOX}/pkg_comp/tmp (+FIX)

[kre%munnari.OZ.AU@localhost]

>Number:         38230
>Category:       pkg
>Synopsis:       pkgtools/pkg_comp leaves turds in ${SANDBOX}/pkg_comp/tmp 
>(+FIX)
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    pkg-manager
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Mar 12 14:25:00 +0000 2008
>Originator:     Robert Elz
>Release:        NetBSD 3.99.15  (pkgsrc current within past 8 hours)
>Organization:
        Prince of Songkla University
>Environment:
System: NetBSD jade.coe.psu.ac.th 3.99.15 NetBSD 3.99.15 
(GENERIC-1.696-20060125) #8: Wed Jan 25 04:59:39 ICT 2006 
kre%jade.coe.psu.ac.th@localhost:/usr/obj/current/kernels/JADE_ASUS i386
Architecture: i386
Machine: i386
>Description:
        pkg_comp leaves empty mktemp(1) files in its sandbox when it
        exits.

>How-To-Repeat:
        Run pkg_comp to build anything...
                pkg_comp build pkgtools/pkg_comp
        would do.

        Then ls -l ${PKG_COMP_SANDBOX_BASEDIR}/pkg_comp/tmp

        Expect to see something like ...

jade$ ls -l
total 0
-rw-------  1 root  wheel  0 Mar 12 18:58 pkg_comp-064a
-rw-------  1 root  wheel  0 Mar 12 18:52 pkg_comp-083a
-rw-------  1 root  wheel  0 Mar 12 18:37 pkg_comp-165a
-rw-------  1 root  wheel  0 Mar 12 18:43 pkg_comp-166a

        (I had many more, but they're all basically the same, there
        should be one for each package build attempt, successful or
        not, since the last "pkg_comp makeroot")

>Fix:
        Apply the appended patch - one use of mktemp(1) was not
        following the pattern applied elsewhere in the script.

        But while I'm here, I don't really see the point of mktemp
        used this way - its intent is to safely create a file the
        script can use - in pkg_comp all the script ever does with
        the created file is remove it.   Given this, mktemp -u would be
        just as good (which isn't really much good), and quicker.

        But all this mess is mostly beause someone seems to believe
        there's some advantage in having shell scripts use names
        that end ".sh" - what's more, these are scripts that (normally)
        no human ever sees, they are created, executed, and removed,
        all quite quickly.

        If pkg_comp were to simply use the results of mktemp(1) as
        the script filename to use, it would all work just as well,
        and be much much safer.  Someone wanting to attack it now,
        can just create the .sh file as a symlink to whatever they
        want to overwrite (since the basename doesn't exist, the
        output from mktemp(1) is highly predictable), mktemp won't notice
        that file exists - true this is not easy in any case as pkg_comp/tmp
        is root owned and 755 mode, but if that's safe enough, then mktemp
        is not achieving anything, and the script might just as well
        create its own names)

        In any case, here's the patch for the forgotten "rm".
        (there's no real need to have the variable "prefix", it
        could have just used "script", but for consistency I just
        copied the pattern used elsewhere).

--- pkg_comp.sh 2007-10-28 07:28:06.000000000 +0700
+++ pkg_comp.sh-FIXED   2008-03-12 20:58:09.000000000 +0700
@@ -764,14 +764,16 @@
 #
 check_pkg_install()
 {
-    local script
+    local script prefix
 
     copy_vulnerabilities
 
     # We assume filesystems are mounted!
 
     echo "PKG_COMP ==> Checking if pkg_install is up to date"
-    script=`mktemp $DESTDIR/pkg_comp/tmp/pkg_comp-XXXX`.sh
+    prefix=`mktemp $DESTDIR/pkg_comp/tmp/pkg_comp-XXXX`
+    rm "${prefix}"
+    script=${prefix}.sh
     init_script $script
     cat >> $script <<EOF
 cd /usr/pkgsrc/pkgtools/pkg_comp