Bug 1085761

Summary: fedora-review incorrect env unset in review-env.sh script
Product: [Fedora] Fedora Reporter: Pavel Alexeev <pahan>
Component: fedora-reviewAssignee: Stanislav Ochotnicky <sochotni>
Status: CLOSED UPSTREAM QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 20CC: fedora, fweimer, leamas.alec, loganjerry, orion, pingou, sochotni
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-04-15 14:03:32 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Pavel Alexeev 2014-04-09 09:48:52 UTC
Description of problem:
$ LANG=C fedora-review -b 1084397
…
WARNING: Illegal return from /usr/share/fedora-review/scripts/generic-large-data.sh, code 80, output: stdout:None stderr:./review-env.sh: line 7: unset: `:DO': not a valid identifier
./review-env.sh: line 7: unset: `:cd': not a valid identifier
./review-env.sh: line 7: unset: `:do': not a valid identifier
./review-env.sh: line 7: unset: `:le': not a valid identifier
./review-env.sh: line 7: unset: `:li#79:co#253:am:xn:xv:LP:sr': not a valid identifier
./review-env.sh: line 7: unset: `:cs': not a valid identifier
./review-env.sh: line 7: unset: `:im': not a valid identifier
./review-env.sh: line 7: unset: `:ke': not a valid identifier
./review-env.sh: line 7: unset: `:ti': not a valid identifier
./review-env.sh: line 7: unset: `:se': not a valid identifier
./review-env.sh: line 7: unset: `:Co#256:pa#64:AF': not a valid identifier
./review-env.sh: line 7: unset: `:vb': not a valid identifier
./review-env.sh: line 7: unset: `:ac': not a valid identifier
./review-env.sh: line 7: unset: `:po': not a valid identifier
./review-env.sh: line 7: unset: `:k3': not a valid identifier
./review-env.sh: line 7: unset: `:k8': not a valid identifier
./review-env.sh: line 7: unset: `:F3': not a valid identifier
./review-env.sh: line 7: unset: `:F7': not a valid identifier
./review-env.sh: line 7: unset: `:K2': not a valid identifier
./review-env.sh: line 7: unset: `:*7': not a valid identifier
./review-env.sh: line 7: unset: `:%e': not a valid identifier
./review-env.sh: line 7: unset: `:@7': not a valid identifier
./review-env.sh: line 7: unset: `:kd': not a valid identifier

It come from line 7 aforementioned script:
unset $(env | sed -n 's/=.*//p')

It does not account what more then one line variables body may be set:
$ env | fgrep -A23 TERMCAP=
TERMCAP=SC|screen|VT 100/ANSI X3.64 virtual terminal:\
        :DO=\E[%dB:LE=\E[%dD:RI=\E[%dC:UP=\E[%dA:bs:bt=\E[Z:\
        :cd=\E[J:ce=\E[K:cl=\E[H\E[J:cm=\E[%i%d;%dH:ct=\E[3g:\
        :do=^J:nd=\E[C:pt:rc=\E8:rs=\Ec:sc=\E7:st=\EH:up=\EM:\
        :le=^H:bl=^G:cr=^M:it#8:ho=\E[H:nw=\EE:ta=^I:is=\E)0:\
        :li#79:co#253:am:xn:xv:LP:sr=\EM:al=\E[L:AL=\E[%dL:\
        :cs=\E[%i%d;%dr:dl=\E[M:DL=\E[%dM:dc=\E[P:DC=\E[%dP:\
        :im=\E[4h:ei=\E[4l:mi:IC=\E[%d@:ks=\E[?1h\E=:\
        :ke=\E[?1l\E>:vi=\E[?25l:ve=\E[34h\E[?25h:vs=\E[34l:\
        :ti=\E[?1049h:te=\E[?1049l:us=\E[4m:ue=\E[24m:so=\E[3m:\
        :se=\E[23m:mb=\E[5m:md=\E[1m:mr=\E[7m:me=\E[m:ms:\
        :Co#256:pa#64:AF=\E[3%dm:AB=\E[4%dm:op=\E[39;49m:AX:\
        :vb=\Eg:G0:as=\E(0:ae=\E(B:\
        :ac=\140\140aaffggjjkkllmmnnooppqqrrssttuuvvwwxxyyzz{{||}}~~..--++,,hhII00:\
        :po=\E[5i:pf=\E[4i:Km=\E[M:k0=\E[10~:k1=\EOP:k2=\EOQ:\
        :k3=\EOR:k4=\EOS:k5=\E[15~:k6=\E[17~:k7=\E[18~:\
        :k8=\E[19~:k9=\E[20~:k;=\E[21~:F1=\E[23~:F2=\E[24~:\
        :F3=\E[1;2P:F4=\E[1;2Q:F5=\E[1;2R:F6=\E[1;2S:\
        :F7=\E[15;2~:F8=\E[17;2~:F9=\E[18;2~:FA=\E[19;2~:kb=:\
        :K2=\EOE:kB=\E[Z:kF=\E[1;2B:kR=\E[1;2A:*4=\E[3;2~:\
        :*7=\E[1;2F:#2=\E[1;2H:#3=\E[2;2~:#4=\E[1;2D:%c=\E[6;2~:\
        :%e=\E[5;2~:%i=\E[1;2C:kh=\E[1~:@1=\E[1~:kH=\E[4~:\
        :@7=\E[4~:kN=\E[6~:kP=\E[5~:kI=\E[2~:kD=\E[3~:ku=\EOA:\
        :kd=\EOB:kr=\EOC:kl=\EOD:km:

Variant which should solve it:
unset $( env | sed -n 's/=.*//p' | egrep -v '^\s' )

Version-Release number of selected component (if applicable):
$ rpm -q fedora-review
fedora-review-0.5.1-2.fc20.noarch

How reproducible:
Always

Comment 1 Alec Leamas 2014-04-09 10:25:20 UTC
Nice catch!

That said, solution seems to either trust continuation lines to be indented as in your proposal, or trust environment variables name to be sane by filtering with something like grep -E '^[a-zA-Z0-9_]+='. None of these solutions are without loopholes, though(?)

Perhaps making an assumption about "sane" environment variables is a little more safe. For one thing, whatever other things there are in the env those will then not be unset nut rather silently ignored. Perhaps better, dunno.

Comment 2 Florian Weimer 2014-10-06 13:15:19 UTC
This also breaks more often with bash-4.2.51-2.fc20.x86_64 due the function name mangling patch.

Comment 3 Alec Leamas 2014-10-14 11:56:39 UTC
Fixed upstream in https://fedorahosted.org/FedoraReview/changeset/18d98aa3d0 Thanks for the 'env -i' hint!

Comment 4 Orion Poplawski 2014-10-30 15:49:33 UTC
Can we get a new release for F20 (and probably F19) then?

Comment 5 Orion Poplawski 2015-01-31 04:36:01 UTC
Also seeing:
WARNING: Illegal return from /usr/share/fedora-review/scripts/java-check-bundled-jars.sh, code 81, output: stdout:Jar files in source (see attachment)
 stderr:./review-env.sh: line 7: unset: `BASH_FUNC_module()': not a valid identifier

fedora-review-0.5.2-2.fc21.noarch

Comment 6 Gerard Ryan 2015-03-01 17:35:55 UTC
(In reply to Orion Poplawski from comment #5)
> [...]
> fedora-review-0.5.2-2.fc21.noarch

I'm also seeing this in this version in F21:

WARNING: Illegal return from /usr/share/fedora-review/scripts/generic-excludearch.sh, code 82, output: stdout:None stderr:./review-env.sh: line 7: unset: `BASH_FUNC_module()': not a valid identifier
./review-env.sh: line 7: unset: `BASH_FUNC_scl()': not a valid identifier
./review-env.sh: line 7: unset: `[': not a valid identifier
./review-env.sh: line 7: unset: `"$CMD"': not a valid identifier

WARNING: Illegal return from /usr/share/fedora-review/scripts/generic-large-docs.sh, code 82, output: stdout:Documentation size is 10240 bytes in 1 files.
 stderr:./review-env.sh: line 7: unset: `BASH_FUNC_module()': not a valid identifier
./review-env.sh: line 7: unset: `BASH_FUNC_scl()': not a valid identifier
./review-env.sh: line 7: unset: `[': not a valid identifier
./review-env.sh: line 7: unset: `"$CMD"': not a valid identifier

WARNING: Illegal return from /usr/share/fedora-review/scripts/generic-srv-opt.sh, code 80, output: stdout:None stderr:./review-env.sh: line 7: unset: `BASH_FUNC_module()': not a valid identifier
./review-env.sh: line 7: unset: `BASH_FUNC_scl()': not a valid identifier
./review-env.sh: line 7: unset: `[': not a valid identifier
./review-env.sh: line 7: unset: `"$CMD"': not a valid identifier

WARNING: Illegal return from /usr/share/fedora-review/scripts/generic-large-data.sh, code 83, output: stdout:None stderr:./review-env.sh: line 7: unset: `BASH_FUNC_module()': not a valid identifier
./review-env.sh: line 7: unset: `BASH_FUNC_scl()': not a valid identifier
./review-env.sh: line 7: unset: `[': not a valid identifier
./review-env.sh: line 7: unset: `"$CMD"': not a valid identifier

Comment 8 Alec Leamas 2015-04-15 14:06:41 UTC
Oops! Not the commits in #comment7, but this one:

https://fedorahosted.org/FedoraReview/changeset/18d98aa3d0f441f7cd4732