Re: pkg/30956

[Chapman Flack <nbgnats%anastigmatix.net@localhost>]

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

From: Chapman Flack <nbgnats%anastigmatix.net@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: pkg/30956
Date: Tue, 09 Aug 2005 19:49:05 -0500

 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.