Opened 8 months ago

Closed 6 months ago

#28007 closed defect (fixed)

shellcheck: scan-build.sh issues

Reported by: rl1987 Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

Shellcheck (​https://github.com/koalaman/shellcheck) finds the following issues:

In scan-build.sh line 37:
EXTRA_CHECKERS="\
^-- SC2034: EXTRA_CHECKERS appears unused. Verify use (or export if used externally).


In scan-build.sh line 44:
NOISY_CHECKERS="\
^-- SC2034: NOISY_CHECKERS appears unused. Verify use (or export if used externally).


In scan-build.sh line 56:
    $CHECKERS \
    ^-- SC2086: Double quote to prevent globbing and word splitting.


In scan-build.sh line 65:
    $CHECKERS \
    ^-- SC2086: Double quote to prevent globbing and word splitting.


In scan-build.sh line 70:
    $CHECKERS $OUTPUTARG \
    ^-- SC2086: Double quote to prevent globbing and word splitting.

Child Tickets

Change History (14)

comment:2 Changed 8 months ago by nickm

Milestone: Tor: 0.3.6.x-final

comment:3 Changed 7 months ago by teor

Status: newneeds_review

comment:4 Changed 7 months ago by dgoulet

Reviewer: catalyst

comment:5 Changed 7 months ago by catalyst

Status: needs_reviewneeds_information

Thanks for working on this! At first glance the changes themselves seem reasonable.

Do we really want to restrict ourselves to platforms with bash, or ones where it might not be in /bin? I'm not sure we have guidelines for this.

comment:6 Changed 7 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:7 Changed 6 months ago by rl1987

IMO it is perfectly reasonable to expect developers to have bash in /bin.

comment:8 Changed 6 months ago by nickm

How bad would it be to allow any /bin/sh here? OTOH for scan-build.sh it doesn't seem to bad to require bash.

comment:9 in reply to:  7 Changed 6 months ago by teor

Replying to rl1987:

IMO it is perfectly reasonable to expect developers to have bash in /bin.

Many BSD systems have bash in /usr/bin, or as an optional extra.

comment:10 Changed 6 months ago by rl1987

Status: needs_informationneeds_review

Reworked the patch to no longer require Bash: https://github.com/torproject/tor/pull/545

comment:11 in reply to:  10 Changed 6 months ago by catalyst

Status: needs_reviewneeds_revision

Replying to rl1987:

Reworked the patch to no longer require Bash: https://github.com/torproject/tor/pull/545

Thanks! I made a small comment on the pull request about word expansion.

comment:12 Changed 6 months ago by rl1987

Status: needs_revisionneeds_review

comment:13 Changed 6 months ago by catalyst

Status: needs_reviewmerge_ready

Thanks for the updated patches! I'm not set up to easily run scan-build, so nickm or someone else properly equipped should smoke test the updated script.

comment:14 Changed 6 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Patches work for me; merged.

Note: See TracTickets for help on using tickets.