spurious apostrophes in PKGNAME_REQD - prereq to pkg/30954

[nbgnats%anastigmatix.net@localhost]

The following reply was made to PR pkg/30954; it has been noted by GNATS.

From: nbgnats%anastigmatix.net@localhost
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: spurious apostrophes in PKGNAME_REQD - prereq to pkg/30954
Date: Tue,  9 Aug 2005 23:03:55 +0000 (UTC)

 >Submitter-Id: net
 >Originator:   Chapman Flack
 >Organization: 
 >Confidential: no
 >Synopsis:     spurious apostrophes in PKGNAME_REQD - prereq to pkg/30954
 >Severity:     serious
 >Priority:     medium
 >Category:     pkg
 >Class:                sw-bug
 >Release:      2.0.0 (irrelevant to issue)
 >Environment:  NetBSD lundestad.anastigmatix.net 2.0 NetBSD 2.0 (lundestad) 
 >#11: Sat Mar  5 14:01:49 EST 2005  
 >xxx@xxx:/usr/src/sys/arch/i386/compile/lundestad i386 (irrelevant to issue)
 >Description:
 The value of PKGNAME_REQD is not directly usable because it usually contains
 spurious apostrophes that have to be stripped out. However, the apostrophes 
were
 added (1.1300) as a workaround for a particular platform that had buggy 
behavior
 in 2003, and if that system is still buggy, the apostrophes will NOT be seen
 when running on that platform (because it will have consumed them as quotes,
 where on a non-buggy system they are part of the literal value). That's even
 worse: users of PKGNAME_REQD now have to treat it as a value that will USUALLY
 BUT NOT ALWAYS have spurious apostrophes, and so now any use of PKGNAME_REQD 
has
 to contain a conditional workaround of the problem caused by the workaround
 created for a buggy platform in 2003. Also, the change reintroduced a a risk of
 surprise from strange IFS values or globbing with unluckily-named files in the
 filesystem that had been fixed in 1.1275 (Sep 2003).
 
 Happily, there is only one package I know of at present that uses PKGNAME_REQD:
 lang/python/extension.mk had to have a workaround added (1.6) when the
 apostrophes appeared in the value. Therefore, fixing this issue would not
 require much effort in backing out workarounds: lang/python/extension.mk would
 need to have 1.6 backed out. However, this issue has greater impact than just
 lang/python, because it blocks pkg/30954, which impairs functionality of
 bin-install.
 
 RECOMMENDATION IN BRIEF:
 
   Revert the line
 
     ${MAKE} ${MAKEFLAGS} $$target ... PKGNAME_REQD=\'$$pkg\';
 
   to its form as of 1.1275:
 
     ${MAKE} ${MAKEFLAGS} $$target ... PKGNAME_REQD="$$pkg";
     
   Back out revision 1.6 of lang/python/extension.mk.
 
   Then, IF the symptom seen building mplayer on Linux in 2003 reappears--it may
   not, if an underlying bug with that platform got fixed in the meantime--I'll
   be happy to help look for the real problem, toward developing a targeted
   workaround or genuine fix. A test Makefile and some debugging suggestions
   were included in http://mail-index.netbsd.org/tech-pkg/2005/03/19/0011.html.
   The test Makefile can be used to test for problems on a suspect platform
   without waiting for a failed package build.
 
 The recommendation is very short and simple (and there's a patch at the end).
 The reason this PR is long is that it might be counterintuitive--there was a
 problem reported, a change was made that seemed to fix it, why go back? I'll 
try
 to show carefully why the fix was an accident and something else should be 
done.
 
 HISTORY AND DETAILS:
 
 From 1.904 to 1.1275 (Jan 2002 to Sep 2003), bsd.pkg.mk had this line in a
 shell command:
 
   ${MAKE} ${MAKEFLAGS} $$target ... PKGNAME_REQD=$$pkg;
 
 In 1.1275 (Sep 2003), jlam added double quotes:
 
   ${MAKE} ${MAKEFLAGS} $$target ... PKGNAME_REQD="$$pkg";
   
 in 1.1296 (Oct 2003), grant changed it to this:
 
   ${MAKE} ${MAKEFLAGS} $$target ... PKGNAME_REQD=\""$$pkg"\";
 
 in 1.1298 (12 days later), agc reverted that change.
 
 in 1.1300 (same day), grant tried it this way (as it is to this day):
 
   ${MAKE} ${MAKEFLAGS} $$target ... PKGNAME_REQD=\'$$pkg\';
 
 
 This PR is concerned with grant's changes, 1.1296 and 1.1300, both of which
 modify the package name, adding literal double quotes or literal apostrophes to
 it, respectively. grant had a reason for trying this: according to the CVS log,
 a build of mplayer on Linux had failed because a required package was named
 nas>=1.4.2, and at some point the name was treated incorrectly as a 
redirection,
 creating a file named =1.4.2.  grant noticed an invocation of make with
 PKGNAME_REQD=nas>=1.4.2 (no quotes), which would certainly have that effect. As
 the line above is the only line in bsd.pkg.mk that sets PKGNAME_REQD, it was
 quite natural and understandable for that line to be the prime suspect.  And
 apparently the change even made the symptom go away on the problem system, so 
it
 must have looked like an open-and-shut case.
 
 The trouble is, the prime suspect has an ironclad defense.
 
 First, notice the two $ signs in $$pkg. This is not a value substituted into 
the
 shell command by make, but a shell parameter being expanded by the shell 
itself.
 The suspect is accused of forming a shell redirection out of the result of a
 parameter expansion, but it is easy to demonstrate that the shell doesn't do
 that. It is deducible from SUSv3, and it is demonstrable in sh, ash, ksh, 
pdksh,
 and bash. It might /seem/ possible in SUSv3, because redirection is said to
 happen after parameter expansion--but it only happens at redirect operators,
 which were tokenized /before/ expansion; any < or > in an expansion result is
 just a character. So says the standard, and it can be confirmed in all the
 shells just listed.
 
 Second, what is the evidence against the suspect? The witness reports seeing an
 invocation of ${MAKE} with PKGNAME_REQD=nas>=1.4.2. But the accused line will
 produce PKGNAME_REQD="$pkg" - that's what you'd see in the output from make,
 and that's what you'd see with ps. You're seeing the shell command (and nbmake
 seems to overwrite its x=y arguments, so you don't see their values in ps
 anyway). If it was in the + line produced by sh -x (make -dx), then that was
 the way it /should/ have looked if the shell was ash or pdksh (which produce
 their + output after quote removal); it would only have been suspicious if the
 shell was bash or real ksh (which produce + output properly quoted).
 
 Third, observe that dependent package names with >= wildcards are pretty
 common in pkgsrc. If the line in question had really been quoted incorrectly
 to avoid shell redirection, the symptom would not have been /one/ package
 building incorrectly on /one/ platform, nearly two years after the line was
 added. It would have been hard to miss.
 
 So, if the ONLY line anywhere in bsd.pkg.mk that invokes ${MAKE} with an
 assignment for PKGNAME_REQD is accused of doing what it can't have done, on the
 basis of evidence it can't have produced, what's left?  Like the man said,
 "whatever remains, however improbable, must be the truth." There must be some
 other place ${MAKE} can be invoked with an assignment for PKGNAME_REQD. And
 there is: any subsequent recursive invocation of ${MAKE} ${MAKEFLAGS}. Once
 PKGNAME_REQD has been assigned on the command line, it goes into .MAKEOVERRIDES
 where, like MAKEFLAGS, it is supposed to be passed automatically and
 transparently to sub-makes. That means the appropriate quoting has to be done
 automatically, and current nbmake does so (with the :Q in
 Main_ExportMAKEFLAGS). My guess is on the problem platform in 2003, there was
 a make, shell, or library involved that differed somehow in the way quoting
 was handled. That was a bug that deserved to be fixed, so another cost of the
 1.1300 change in bsd.pkg.mk is it probably masked a problem that could
 otherwise have been tracked down.
 
 THE REAL DIFFERENCES BETWEEN THE FOUR VERSIONS OF THE LINE:
 
 All four historical versions have been safe from I/O redirection
 surprises--even the first one, with no quoting at all. As explained above,
 the shell does not see redirection operators in expansion results.
 
 The unquoted first version would be vulnerable to (rare) surprises from
 field splitting (if IFS happened to contain a character that was in the
 package name), or pathname expansion (if the package name contained shell
 wildcards and a file existed with an unlikely name PKGNAME_REQD=foo... that
 happened to match the entire word).  Both cases were unlikely, but jlam's
 fix in 1.1275 made both impossible.
 
 The version with double quotes in 1.1296 should have had no effect except to
 include literal double quotes in the package name. However, agc reverted it,
 apparently in response to breakage (ironically, seems from the CVS log it 
caused
 the same symptom on good platforms it had been intended to correct on bad 
ones).
 The reason it caused breakage probably can be traced to these two lines:
 
   3614:             pkg="${dep:C/:.*//}";
   3615:             dir="${dep:C/[^:]*://:C/:.*$//}";
 
 where the literal doublequotes in the value cancelled the enclosing
 doublequotes, leaving the > character exposed. That would not have happened
 with this form:
 
   3614:             pkg=${dep:C/:.*//:Q};
   3615:             dir=${dep:C/[^:]*://:C/:.*$//:Q};
 
 so I include that change in the patch.
 
 The version with single quotes in 1.1300 avoided that particular accident (just
 because it added single instead of double quotes), but also--because it does
 not in fact quote the value at all--was a regression of jlam's fix in 1.1275,
 reopening the possibility of IFS and glob surprises.
 
 A MAKEFILE FOR EXPLORING THE EFFECTS OF THE FOUR VERSIONS:
 
 This Makefile creates some evilly-named files in the current directory,
 to show the effect of the forms that are not globbing-protected.
 
 all : 904 1275 1296 1300
 
 904 : prepare
        pkg='abc>=1.2.*' ;\
        sh -c 'echo "$$@"' dummy PKGNAME_REQD=$$pkg
 
 1275 : prepare
        pkg='abc>=1.2.*' ;\
        sh -c 'echo "$$@"' dummy PKGNAME_REQD="$$pkg"
 
 1296 : prepare
        pkg='abc>=1.2.*' ;\
        sh -c 'echo "$$@"' dummy PKGNAME_REQD=\""$$pkg"\"
 
 1300 : prepare
        pkg='abc>=1.2.*' ;\
        sh -c 'echo "$$@"' dummy PKGNAME_REQD=\'$$pkg\'
 
 prepare :
        >PKGNAME_REQD=abc\>=1.2.x
        >PKGNAME_REQD=\"abc\>=1.2.x\"
        >PKGNAME_REQD=\'abc\>=1.2.x\'
 
 test :
        pkg='abc>=1.2.*' ;\
        ${MAKE} ${MAKEFLAGS} nothing PKGNAME_REQD="$$pkg"
 
 nothing :
        echo ${PKGNAME_REQD:Q}
        ps -ww -o args
        env
 
 >How-To-Repeat:
 With make -dv, can confirm that PKGNAME_REQD value contains apostrophes when 
bin-install invoked for dependencies (can be masked by pkg/30928--fix that to 
make sure bin-install GETS invoked for dependencies).
 
 See http://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/lang/python/extension.mk#rev1.6 
for example of package rework required by the insertion of spurious apostrophes.
 >Fix:
 PATCH FOR bsd.pkg.mk:
 
 --- /tmp/bsd.pkg.mk    2005-08-09 14:32:15.000000000 -0500
 +++ /tmp/bsd.pkg.mk    2005-08-09 16:53:46.000000000 -0500
 @@ -3611,8 +3611,8 @@
  .    else     # !DEPENDS
  .      for dep in ${_DEPENDS_AND_BUILD_DEPENDS:O}
        ${_PKG_SILENT}${_PKG_DEBUG}                                     \
 -      pkg="${dep:C/:.*//}";                                           \
 -      dir="${dep:C/[^:]*://:C/:.*$//}";                               \
 +      pkg=${dep:C/:.*//:Q};                                          \
 +      dir=${dep:C/[^:]*://:C/:.*$//:Q};                              \
        found=`${PKG_BEST_EXISTS} "$$pkg" || ${TRUE}`;                  \
        if [ "X$$REBUILD_DOWNLEVEL_DEPENDS" != "X" ]; then              \
                pkgname=`cd $$dir ; ${MAKE} ${MAKEFLAGS} show-var 
VARNAME=PKGNAME`; \
 @@ -3644,7 +3644,7 @@
                        ${ECHO_MSG} "=> No directory for $$dir.  Skipping.."; \
                else                                                    \
                        cd $$dir ;                                      \
 -                      ${SETENV} _PKGSRC_DEPS=", ${PKGNAME}${_PKGSRC_DEPS}" 
${MAKE} ${MAKEFLAGS} $$target PKGNAME_REQD=\'$$pkg\' MAKEFLAGS="" || exit 1; \
 +                      ${SETENV} _PKGSRC_DEPS=", ${PKGNAME}${_PKGSRC_DEPS}" 
${MAKE} ${MAKEFLAGS} $$target PKGNAME_REQD="$$pkg" MAKEFLAGS="" || exit 1; \
                        ${ECHO_MSG} "${_PKGSRC_IN}> Returning to build of 
${PKGNAME}"; \
                fi;                                                     \
        fi
 
 THEN REVERT r1.6 of lang/python/extension.mk.
 
 The original problem on some platform that the apostrophes were an attempt to 
fix *may* reappear; if it does, I'll be happy to help find the real cause and 
solution. Some debug ideas, if necessary, in 
http://mail-index.netbsd.org/tech-pkg/2005/03/19/0011.html