From 078758a08b5dcaff399bcd1e13b10ef7e3aa139b Mon Sep 17 00:00:00 2001 From: Thomas Bracht Laumann Jespersen Date: Wed, 18 May 2022 14:43:25 +0200 Subject: [PATCH] new check: variables that should be quoted This is based on the repoman check EbuildQuote that reports instances of some variables that should be quoted in certain contexts. See: https://gitweb.gentoo.org/proj/portage.git/tree/repoman/lib/repoman/modules/linechecks/quotes/quotes.py?h=portage-3.0.30 Closes: https://github.com/pkgcore/pkgcheck/issues/363 Signed-off-by: Thomas Bracht Laumann Jespersen --- src/pkgcheck/checks/codingstyle.py | 115 ++++++++++++++++++ .../EclassUnquotedVariable/expected.json | 1 + .../EbuildUnquotedVariable/expected.json | 3 + .../EbuildUnquotedVariable/fix.patch | 46 +++++++ .../eclass/eclass/unquotedvariable.eclass | 19 +++ .../EbuildUnquotedVariable-0.ebuild | 43 +++++++ 6 files changed, 227 insertions(+) create mode 100644 testdata/data/repos/eclass/EclassUnquotedVariablesCheck/EclassUnquotedVariable/expected.json create mode 100644 testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/expected.json create mode 100644 testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/fix.patch create mode 100644 testdata/repos/eclass/eclass/unquotedvariable.eclass create mode 100644 testdata/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/EbuildUnquotedVariable-0.ebuild diff --git a/src/pkgcheck/checks/codingstyle.py b/src/pkgcheck/checks/codingstyle.py index 56e0f7ece..595d5f28c 100644 --- a/src/pkgcheck/checks/codingstyle.py +++ b/src/pkgcheck/checks/codingstyle.py @@ -903,3 +903,118 @@ def feed(self, pkg): yield RedundantDodir( cmd.group('cmd'), line=dodir.group('call'), lineno=lineno - 1, pkg=pkg) + + +class UnquotedVariable(results.AliasResult, results.Warning): + """Variable is used unquoted in a context where it should be quoted. + + Variables like D, FILESDIR, etc may not be safe to use unquoted in some + contexts. + """ + + _name = 'UnquotedVariable' + + def __init__(self, variable, lines, **kwargs): + super().__init__(**kwargs) + self.variable = variable + self.lines = tuple(lines) + + @property + def desc(self): + s = pluralism(self.lines) + lines = ', '.join(map(str, self.lines)) + return f'unquoted variable {self.variable} on line{s}: {lines} ' + + + +class EbuildUnquotedVariable(UnquotedVariable, results.VersionResult): + __doc__ = UnquotedVariable.__doc__ + + +class EclassUnquotedVariable(UnquotedVariable, results.EclassResult): + __doc__ = UnquotedVariable.__doc__ + + @property + def desc(self): + return f'{self.eclass}: {super().desc}' + + +class _UnquotedVariablesCheck(Check): + """Scan files for variables that should be quoted like D, FILESDIR, etc.""" + + message_commands = frozenset({ + "die", "echo", "eerror", "einfo", "elog", "eqawarn", "ewarn" + }) + var_names = frozenset({ + "D", "DISTDIR", "FILESDIR", "S", "T", "ROOT", "BROOT", "WORKDIR", "ED", + "EPREFIX", "EROOT", "SYSROOT", "ESYSROOT", "TMPDIR", "HOME", + # variables for multibuild.eclass + "BUILD_DIR", + }) + + node_types_ok = frozenset({ + # Variable is sitting in a string, all good + 'string', + # Variable is part of a shell assignment, and does not need to be + # quoted. for example S=${WORKDIR}/${PN} is ok. + 'variable_assignment', + # Variable sits inside a [[ ]] test command and it's OK not to be quoted + 'test_command', + # Variable is being used in a heredoc body, no need to specify quotes. + 'heredoc_body', + }) + + def _var_needs_quotes(self, pkg, node): + pnode = node.parent + while pnode != node: + if pnode.type in self.node_types_ok: + return False + elif pnode.type == 'command': + cmd = pkg.node_str(pnode.child_by_field_name('name')) + return cmd not in self.message_commands + elif pnode.type in 'array': + # Variable is sitting unquoted in an array + return True + pnode = pnode.parent + + # Default: The variable should be quoted + return True + + def _feed(self, item): + if item.tree.root_node.has_error: + # Do not run this check if the parse tree contains errors, as it + # might result in false positives. This check appears to be quite + # expensive though... + return + hits = defaultdict(set) + for var_node, _ in bash.var_query.captures(item.tree.root_node): + var_name = item.node_str(var_node) + if var_name in self.var_names: + if self._var_needs_quotes(item, var_node): + lineno, _ = var_node.start_point + hits[var_name].add(lineno+1) + for var_name, lines in hits.items(): + yield var_name, sorted(lines) + + +class EbuildUnquotedVariablesCheck(_UnquotedVariablesCheck): + """Scan ebuild for variables that should be quoted like D, FILESDIR, etc.""" + + _source = sources.EbuildParseRepoSource + known_results = frozenset([EbuildUnquotedVariable]) + + def feed(self, pkg): + for var_name, lines in self._feed(pkg): + yield EbuildUnquotedVariable(var_name, lines, pkg=pkg) + + +class EclassUnquotedVariablesCheck(_UnquotedVariablesCheck): + """Scan eclass for variables that should be quoted like D, FILESDIR, etc.""" + + _source = sources.EclassParseRepoSource + known_results = frozenset([EclassUnquotedVariable]) + required_addons = (addons.eclass.EclassAddon,) + + def feed(self, eclass): + for var_name, lines in self._feed(eclass): + yield EclassUnquotedVariable(var_name, lines, eclass=eclass.name) diff --git a/testdata/data/repos/eclass/EclassUnquotedVariablesCheck/EclassUnquotedVariable/expected.json b/testdata/data/repos/eclass/EclassUnquotedVariablesCheck/EclassUnquotedVariable/expected.json new file mode 100644 index 000000000..4ac220eee --- /dev/null +++ b/testdata/data/repos/eclass/EclassUnquotedVariablesCheck/EclassUnquotedVariable/expected.json @@ -0,0 +1 @@ +{"__class__": "EclassUnquotedVariable", "eclass": "unquotedvariable", "variable": "DISTDIR", "lines": [19]} diff --git a/testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/expected.json b/testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/expected.json new file mode 100644 index 000000000..ab08e2bee --- /dev/null +++ b/testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/expected.json @@ -0,0 +1,3 @@ +{"__class__": "EbuildUnquotedVariable", "category": "EbuildUnquotedVariablesCheck", "package": "EbuildUnquotedVariable", "version": "0", "variable": "FILESDIR", "lines": [9, 19]} +{"__class__": "EbuildUnquotedVariable", "category": "EbuildUnquotedVariablesCheck", "package": "EbuildUnquotedVariable", "version": "0", "variable": "T", "lines": [25, 27, 41, 42]} +{"__class__": "EbuildUnquotedVariable", "category": "EbuildUnquotedVariablesCheck", "package": "EbuildUnquotedVariable", "version": "0", "variable": "WORKDIR", "lines": [34]} diff --git a/testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/fix.patch b/testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/fix.patch new file mode 100644 index 000000000..a89ad5c40 --- /dev/null +++ b/testdata/data/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/fix.patch @@ -0,0 +1,46 @@ +--- standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/EbuildUnquotedVariable-0.ebuild 2022-05-18 20:27:34.657647175 +0200 ++++ fixed/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/EbuildUnquotedVariable-0.ebuild 2022-05-18 20:50:00.271294657 +0200 +@@ -6,7 +6,7 @@ + S=${WORKDIR}/${PV} # ok + + PATCHES=( +- ${FILESDIR}/foo.patch # FAIL ++ "${FILESDIR}"/foo.patch # FAIL + "${FILESDIR}"/foo.patch # ok + "${FILESDIR}/foo.patch" # ok + ) +@@ -16,28 +16,28 @@ + : + fi + +- if has "foo" ${FILESDIR} ; then # FAIL ++ if has "foo" "${FILESDIR}" ; then # FAIL + : + fi + + local t=${T} # ok + local t=${T}/t # ok +- emake CC=${T}; someotherfunc ${T} # FAIL ++ emake CC="${T}"; someotherfunc "${T}" # FAIL + +- local var=( TMP=${T} ) # FAIL ++ local var=( TMP="${T}" ) # FAIL + + cat > "${T}"/somefile <<- EOF || die + PATH="${EPREFIX}${INSTALLDIR}/bin" + EOF + doenvd "${T}"/10stuffit # ok + +- echo "InitiatorName=$(${WORKDIR}/usr/sbin/iscsi-iname)" # FAIL ++ echo "InitiatorName=$("${WORKDIR}"/usr/sbin/iscsi-iname)" # FAIL + einfo ${WORKDIR} # ok + + if grep -qs '^ *sshd *:' "${WORKDIR}"/etc/hosts.{allow,deny} ; then # ok + ewarn "something something ${WORKDIR} here" + fi + +- cat < ${T} # FAIL +- cat >> ${T} # FAIL ++ cat < "${T}" # FAIL ++ cat >> "${T}" # FAIL + } diff --git a/testdata/repos/eclass/eclass/unquotedvariable.eclass b/testdata/repos/eclass/eclass/unquotedvariable.eclass new file mode 100644 index 000000000..e72b75eed --- /dev/null +++ b/testdata/repos/eclass/eclass/unquotedvariable.eclass @@ -0,0 +1,19 @@ +# Copyright 2022 Gentoo Authors +# Distributed under the terms of the GNU General Public License v2 + +# @ECLASS: unquotedvariable.eclass +# @MAINTAINER: +# Larry the Cow +# @AUTHOR: +# Larry the Cow +# @SUPPORTED_EAPIS: 8 +# @BLURB: Eclass that forgets to quote a variable properly +# @DESCRIPTION: +# A small eclass that defines a single variable, but references a variable that +# needs to be quoted in specific contexts. + +# @ECLASS_VARIABLE: EBZR_STORE_DIR +# @USER_VARIABLE +# @DESCRIPTION: +# The directory to store all fetched Bazaar live sources. +: ${EBZR_STORE_DIR:=${PORTAGE_ACTUAL_DISTDIR:-${DISTDIR}}/bzr-src} diff --git a/testdata/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/EbuildUnquotedVariable-0.ebuild b/testdata/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/EbuildUnquotedVariable-0.ebuild new file mode 100644 index 000000000..83a381bf9 --- /dev/null +++ b/testdata/repos/standalone/EbuildUnquotedVariablesCheck/EbuildUnquotedVariable/EbuildUnquotedVariable-0.ebuild @@ -0,0 +1,43 @@ +EAPI=8 +DESCRIPTION="Ebuild with variables that must be quoted" +HOMEPAGE="https://github.com/pkgcore/pkgcheck" +SLOT="0" +LICENSE="BSD" +S=${WORKDIR}/${PV} # ok + +PATCHES=( + ${FILESDIR}/foo.patch # FAIL + "${FILESDIR}"/foo.patch # ok + "${FILESDIR}/foo.patch" # ok +) + +src_prepare() { + if [[ -z ${S} ]]; then # ok + : + fi + + if has "foo" ${FILESDIR} ; then # FAIL + : + fi + + local t=${T} # ok + local t=${T}/t # ok + emake CC=${T}; someotherfunc ${T} # FAIL + + local var=( TMP=${T} ) # FAIL + + cat > "${T}"/somefile <<- EOF || die + PATH="${EPREFIX}${INSTALLDIR}/bin" + EOF + doenvd "${T}"/10stuffit # ok + + echo "InitiatorName=$(${WORKDIR}/usr/sbin/iscsi-iname)" # FAIL + einfo ${WORKDIR} # ok + + if grep -qs '^ *sshd *:' "${WORKDIR}"/etc/hosts.{allow,deny} ; then # ok + ewarn "something something ${WORKDIR} here" + fi + + cat < ${T} # FAIL + cat >> ${T} # FAIL +}