Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 848708 - net-dns/bind-9.16.33 installs shell script that uses non-POSIX features
Summary: net-dns/bind-9.16.33 installs shell script that uses non-POSIX features
Status: CONFIRMED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Mikle Kolyada
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 609070
  Show dependency tree
 
Reported: 2022-05-31 06:57 UTC by Agostino Sarubbo
Modified: 2022-09-23 11:18 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
build.log.xz (build.log.xz,36.68 KB, application/x-xz)
2022-05-31 06:57 UTC, Agostino Sarubbo
Details
gentoo-bug-848708.patch (gentoo-bug-848708.patch,6.03 KB, patch)
2022-06-30 23:31 UTC, Kerin Millar
Details | Diff
gentoo-bug-848708-r1.patch (gentoo-bug-848708-r1.patch,5.98 KB, patch)
2022-07-01 08:07 UTC, Kerin Millar
Details | Diff
gentoo-bug-848708-r2.patch (gentoo-bug-848708-r2.patch,5.97 KB, patch)
2022-07-01 08:17 UTC, Kerin Millar
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Agostino Sarubbo gentoo-dev 2022-05-31 06:57:54 UTC
https://blogs.gentoo.org/ago/2020/07/04/gentoo-tinderbox/

Issue: net-dns/bind-9.16.29 installs shell script that uses non-POSIX features.
Discovered on: amd64 (internal ref: ci)
Comment 1 Agostino Sarubbo gentoo-dev 2022-05-31 06:57:57 UTC
Created attachment 781553 [details]
build.log.xz

build log and emerge --info (compressed because it exceeds attachment limit, use 'xzless' to read it)
Comment 2 Agostino Sarubbo gentoo-dev 2022-06-03 14:16:11 UTC
ci has reproduced this issue with version 9.16.29-r1 - Updating summary.
Comment 3 Agostino Sarubbo gentoo-dev 2022-06-17 06:48:13 UTC
ci has reproduced this issue with version 9.16.30 - Updating summary.
Comment 4 Kerin Millar 2022-06-30 22:01:59 UTC
This is INVALID. Though the script could use a clean-up, the two QA notices in the build log are false positives. The only POSIX violation is the ever-pervasive use of local.
Comment 5 Kerin Millar 2022-06-30 22:04:37 UTC
I don't feel like re-writing the whole thing right now but I'll attach a patch shortly that improves those two tests and should keep checkbashisms quiet.
Comment 6 Kerin Millar 2022-06-30 23:31:31 UTC
Created attachment 789110 [details, diff]
gentoo-bug-848708.patch

This is a direct patch against named.init-r14 so as to make the changes easier to review. My proposed commit message is as follows ...

Rename _mount to _bindmount and remove the needless option support. The
only options ever being passed prior were -o bind. Remove the useless
$# check; there are always a sufficient number of arguments. Remove the
useless intermediate variable that was holding the value of $?; the
eend function already propagates the status code that is given, so one
may simply let it fall through to an implicit return. Remove the
useless return 0; if already produces a status of 0 in the case that
the given command(s) fail. Use mountpoint(1) from util-linux to
determine whether the mountpoint exists. Doing so inhibits a spurious
warning raised by checkbashisms into the bargain. Resolve a dangling
eend by calling ebegin instead of einfo.

Apply the aforementioned changes to _umount, where applicable.

Do not use the -a operator in tests, as its use is discouraged.

Do not initialise the 'reported' variable at the same that it is
localised. The effect of using local is still undefined by POSIX, so
that's asking for trouble.

Fix almost all cases in which expansions are erroneously unquoted
(SC2086).

Closes: https://bugs.gentoo.org/848708
Signed-off-by: Kerin Millar <kfm@plushkava.net>
Comment 7 Kerin Millar 2022-07-01 08:07:57 UTC
Created attachment 789149 [details, diff]
gentoo-bug-848708-r1.patch

Upon self-review of my patch, I realised that I had accidentally double-quoted the expansion of CHROOT in the invocation of start-stop-daemon. Unfortunately, the present approach to argument composition requires that it be unquoted. For now, make it so. No other changes.
Comment 8 Kerin Millar 2022-07-01 08:17:21 UTC
Created attachment 789152 [details, diff]
gentoo-bug-848708-r2.patch

Take into account three more cases where CHROOT must specifically remain unquoted, this time involving invocations of named-checkconf. Third time should be the charm.
Comment 9 Agostino Sarubbo gentoo-dev 2022-07-01 08:24:32 UTC
(In reply to Kerin Millar from comment #4)
> This is INVALID.

If you think this is invalid, the best approach is send improvements to checkbashism or filter the invalid notice in portage.
Comment 10 Kerin Millar 2022-07-01 09:10:04 UTC
(In reply to Agostino Sarubbo from comment #9)
> (In reply to Kerin Millar from comment #4)
> > This is INVALID.
> 
> If you think this is invalid, the best approach is send improvements to
> checkbashism or filter the invalid notice in portage.

checkbashisms isn't especially adept in its capacity as a shell code parser and is essentially treating the == operator here as if it is shellcode.

[ -n "$(awk 'this == that')" ]

That is not to say that checkbashisms isn't useful, of course. It's high time that the issue of pervasive POSIX violations was attended to and your initiative is welcome.

I do not have any personal interest in attempting to improve chekbashisms itself, because shellcheck is already a much better parser and is already capable of extensively flagging POSIX violations, though there is presently no simply option to make it exclusively report violations in that category. The word "best" can mean different things to different people and it is my view that efforts are better directed towards the shellcheck project. Besides, even the very best linting tools can - and will - produce false positives in some context.

I do, however, have an interest in hacking on shell code that could stand to be improved and have made prior contributions in that vein. The _mount and _unmount routines are convoluted and easy to improve. Even if mountpoint(1) did not exist as a utility, there's no need to use a command substitution, nor awk itself. For example, this routine would be more correct than the existing code (it decodes the second field correctly) and would not offend checkbashisms.

while read -r _ target _; do
	target=$(printf %b "$target")
	if [ "$to" = "$target" ]; then
		return
	fi
done < /proc/self/mounts
# mount stuff here

But mountpoint(1) does exist so why not use it?

Not all effort is fungible. While I understand your point, a lack of interest in working on checkbashisms should not preclude me from making what I believe to be useful suggestions that can result in bugs being closed, including this one.
Comment 11 Agostino Sarubbo gentoo-dev 2022-07-21 06:46:55 UTC
ci has reproduced this issue with version 9.16.31 - Updating summary.
Comment 12 Agostino Sarubbo gentoo-dev 2022-08-18 06:39:06 UTC
ci has reproduced this issue with version 9.16.32 - Updating summary.
Comment 13 Agostino Sarubbo gentoo-dev 2022-09-23 11:18:53 UTC
ci has reproduced this issue with version 9.16.33 - Updating summary.