user devscri...@packages.debian.org usertags 911969 cvs-deb cvs-debrelease deb-reversion debsign dscextract getbuildlog mergechanges pts-subscribe wnpp-alert wnpp-check thanks
Hi,
On Fri, Oct 26, 2018 at 08:54:36PM +0200, Jakub Wilk wrote:
> I've found quite a few bugs similar to #911720 in devscripts codebase. Here
> are excerpts of buggy code (with boring parts omitted):
I think none of the below have the chance to be as serious as #911720,
but thanks for compiling the list and doing the initial investigation.
> * cvs-debi, cvs-debrelease:
>
> TEMPDIR=$(mktemp -dt cvs-debi.XXXXXXXX) || ...
> TEMPFILE=$TEMPDIR/cl-tmp
> trap "rm -f $TEMPFILE; rmdir $TEMPDIR" 0 1 2 3 7 10 13 15
>
> * deb-reversion:
>
> TMPDIR=$(mktemp -d --tmpdir deb-reversion.XXXXXX)
> trap "rm -rf $TMPDIR" 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
>
> * debsign:
>
> trap "cleanup_tmpdir" EXIT HUP INT QUIT KILL SEGV PIPE TERM
For these I simply made them to catch only EXIT, without any special
handling for all the others signals. Also, I fixed the quoting…
> mksigningdir () {
> ...
> signingdir="$(mktemp -dt debsign.XXXXXXXX)" || ...
> ...
> }
> mkremotefilesdir () {
> ...
> remotefilesdir="$(mktemp -dt debsign.XXXXXXXX)" || ...
> ...
> }
> ...
> cleanup_tmpdir () {
> ... # removes $remotefilesdir and $signingdir, but doesn't exit
> }
why do you say so? ITSM cleanup_tmpdir() properly checks for the
presence of the directory before doing anything?
> * dscextract:
>
> WORKDIR=$(mktemp -d --tmpdir dscextract.XXXXXX)
> trap "rm -rf $WORKDIR" 0 2 3 15
>
> * getbuildlog:
>
> ALL_LOGS=`mktemp`
> trap "rm -f $ALL_LOGS" EXIT INT QUIT TERM
>
> * mergechanges:
>
> OUTPUT=`tempfile`
> DESCFILE=`tempfile`
> trap "rm -f '${OUTPUT}' '${DESCFILE}'" 0 1 2 3 7 10 13 15
>
> * pts-subscribe:
>
> TEMPFILE=$(mktemp) || ...
> trap "rm -f '$TEMPFILE'" 0 1 2 3 7 10 13 15
also here, made the trap only trap EXIT and fixed quoting and mktemp
flags…
also moved away from tempfile(1) to mktemp(1) and added prefix
indicating the program that created the temp files.
> * wnpp-alert:
>
> INSTALLED=`mktemp -t wnppalert-installed.XXXXXX`
> trap "rm -f '$INSTALLED'" 0 1 2 3 7 10 13 15
> WNPP=`mktemp -t wnppalert-wnpp.XXXXXX`
> WNPPTMP=`mktemp -t wnppalert-wnpp.XXXXXX`
> trap "rm -f '$INSTALLED' '$WNPP' '$WNPPTMP'" 0 1 2 3 7 10 13 15
> WNPP_PACKAGES=`mktemp -t wnppalert-wnpp_packages.XXXXXX`
> trap "rm -f '$INSTALLED' '$WNPP' '$WNPPTMP' '$WNPP_PACKAGES'" \
> 0 1 2 3 7 10 13 15
>
> ...
>
> WNPP_DIFF=`mktemp -t wnppalert-wnpp_diff.XXXXXX`
> trap "rm -f '$INSTALLED' '$WNPP' '$WNPPTMP' '$WNPP_PACKAGES'
> '$WNPP_DIFF'" \
> 0 1 2 3 7 10 13 15
wow how messy…
I just changed them to a single trap at end. Theoretically this could
be bad in the case a mktemp fail midway and scripts exit, but if mktemp
fails that badly I think you shouldn't worry about a stray tempfile
anyway…
> * wnpp-check:
>
> WNPP=`mktemp -t wnppcheck-wnpp.XXXXXX`
> WNPPTMP=`mktemp -t wnppcheck-wnpp.XXXXXX`
> trap "rm -f '$WNPP' '$WNPPTMP'" 0 1 2 3 7 10 13 15
> WNPP_PACKAGES=`mktemp -t wnppcheck-wnpp_packages.XXXXXX`
> trap "rm -f '$WNPP' '$WNPPTMP' '$WNPP_PACKAGES'" \
> 0 1 2 3 7 10 13 15
ditto.
I'd appreciate if you could have a look at my commit and tell me if you
see anything off.
--
regards,
Mattia Rizzolo
GPG Key: 66AE 2B4A FCCF 3F52 DA18 4D18 4B04 3FCD B944 4540 .''`.
more about me: https://mapreri.org : :' :
Launchpad user: https://launchpad.net/~mapreri `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia `-
signature.asc
Description: PGP signature
