Interesting idea. The problem is that brp's do not know anything about RPM, they are just shell scripts which know RPM_BUILD_ROOT and nothing else...

> Why only the same set of shebangs as the set picked up by RPM dependency generators is not adjusted?

What's the set you talk about?  %buildroot/%_bindir?  That would make
sense to me.

Thanks for the report. For the time being disable the mangling as a workaround.

I'll get to this if nobody else does it, but not sooner than 2018-02-14 (vacation).

Yeah in a perfect (or at least better) world build root policies would run according to what's in the buildroot rather than static set of scripts that are always executed, since we already classify the contents anyway. But that's a longer term upstream RFE.

IMHO we need a general purpose path based opt-out, not some heuristic "is this going to be picked by the dependency generator", see bug 1542436.

(In reply to Miro Hrončok from comment #5)
> IMHO we need a general purpose path based opt-out, not some heuristic "is
> this going to be picked by the dependency generator", see bug 1542436.

Opt out is just remediation of the issue, but it should not mangle shebangs, which looks suspicious. Better to fail then provide incorrect result.

IOW, only the shebangs which match this pattern should be mangled /#![a-zA-Z\/]*env [a-zA-Z-]*/ (eventually, it can be even stricter) and for the rest of the cases, the script should just fail.

(In reply to Vít Ondruch from comment #6)
> /#![a-zA-Z\/]*env [a-zA-Z-]*/

Actually numbers should be accepted ...

Yet another reincarnation of similar issue:

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/RYN7K7EP6UFZNBE7ZUQ4EPVQU6GHRLJ6/

This should be resolved prior everybody disables the mangling.

(In reply to Vít Ondruch from comment #8)
> Yet another reincarnation of similar issue:
> 
> https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/
> message/RYN7K7EP6UFZNBE7ZUQ4EPVQU6GHRLJ6/
> 
> This should be resolved prior everybody disables the mangling.

note that I've fixed that case

So looking more into rubygem's failure...

#!./foo/bar depends on current directory, so we should really avoid packaging such files. Either there should be absolute path or executable bit should not be set.

`.` means current directory and not directory of the executable file.

(In reply to Igor Gnatenko from comment #9)
> note that I've fixed that case

Thx.

Anyway there is still this unmerged PR:

https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/16

If the PR gets merged (it should be merged, since it fixes bug), then there is going to be another cases of shebangs which should not be touched.

I think that the script should mange only shebangs it understands. It should not mangle the rest. Providing wrong shebangs is unacceptable. At the minimum, the script should not touch shebangs which does not match following RegExp:

~~~
^#![[:alnum:][:space:]./-]*$
~~~

How to disable or exclude .rs files from this exactly?
We're having problem with Firefox and rust source files, like this: 
https://searchfox.org/mozilla-central/source/third_party/rust/itertools/src/lib.rs

Rust use #![var] as C for #ifndef var:
https://doc.rust-lang.org/book/first-edition/documentation.html#doc-attributes

(In reply to Jan Horak from comment #12)
> How to disable or exclude .rs files from this exactly?

chmod -x! And submit patch to upstream.

> We're having problem with Firefox and rust source files, like this: 
> https://searchfox.org/mozilla-central/source/third_party/rust/itertools/src/
> lib.rs
> 
> Rust use #![var] as C for #ifndef var:
> https://doc.rust-lang.org/book/first-edition/documentation.html#doc-
> attributes

Whatever it uses is not valid shebang and file must not be execurable.

Also see https://github.com/bluss/rust-itertools/commit/c7ea0d52b75c459db8094d98737f631c020b004d for your particular case.

Hm, cool, but when your pull request will be merged:
1 - it will take some time before the third party lib gets updated in mozilla code base
2 - it will be pulled only to nightly branch
3 - it will take 12 weeks to get into stable

I'm I suppose to manually change perms or what?

(In reply to Jan Horak from comment #15)
> Hm, cool, but when your pull request will be merged:

It's merged 3 months ago.

> I'm I suppose to manually change perms or what?

Yes.

JFYI: A packager can now exclude certain files and/or certain shebangs from the mangling: https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/19

Jan Horak: Note that excluding the .rs file (or all of them) via the new macro is not a proper fix, proper fix is to chmod -x it.

As for the bug as it was reported: It's the "shebangs (not) picked up by dependency generators" documented somewhere, or should I just read the source? I always thought it picks them all, but apparently not.

If it only picks absolute paths, than I guess the proper fix is not to have relative shebangs at all, in my eyes a relative shebang is bogus.

As said in comment #10:
> #!./foo/bar depends on current directory, so we should really avoid
> packaging such files. Either there should be absolute path or executable bit
> should not be set.

Originally, I proposed to not mangle shebangs not picked up by RPM, but now I am worried about mangling shebangs like this [1] (@ignatenko and that is why I changed the subject):

~~~
#!/usr/bin/env <%= Bundler.settings[:shebang] || RbConfig::CONFIG["ruby_install_name"] %>
~~~

no matter where they are, the mangling should fail with such shebang, because it is apparently special case. I caught this specific issue just because there was bug in shebang mangling script [2].


[1] https://github.com/bundler/bundler/blob/master/lib/bundler/templates/Executable#L1
[2] https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/16

What do you think by "should fail"? Don't mangle the shebang because it looks weird (has weird symbols in it, etc.), or fail hard because it looks weird?


My opinion:

That file is a template. I think templates with shebangs are "special enough" for the packager to take care of them. Either don't have it executable and the mangling script (and the dep generator) will ignore it or (if that's not possible) exclude it manually from the shebang mangling and the dep generator.


Technically this shebang **is** picked up by the generator (if executable), it's just that only the part before the first space is used as a dependency.


I see a benefit in failing hard if the the shebang is illegal. It would make the packager care.

> I see a benefit in failing hard if the the shebang is illegal. It would make the packager care.

This makes a lot of sense for me. So packager would need to decide what to do with that.

(In reply to Miro Hrončok from comment #19)
> What do you think by "should fail"? Don't mangle the shebang because it
> looks weird (has weird symbols in it, etc.), or fail hard because it looks
> weird?

Fail hard and let packager to decide.

This bug appears to have been reported against 'rawhide' during the Fedora 28 development cycle.
Changing version to '28'.

I just wanted to note this _somewhere_: It is valid for an executable to have no shebang line; the kernel just passes it to /bin/sh.  So removing the executable bits in that case can actually be problematic.  I found some instances of that breakage in a package review and it turns out that the following does disable the "empty shebang" check:

%global __brp_mangle_shebangs_exclude ^$

But that may be purely by accident.

For background, I found that "implicit /bin/sh" bit used in the texlive sources, where this absolutely hideous construct is used to make a shell script which is both valid sh and perl:

eval '(exit $?0)' && eval 'exec perl -S $0 ${1+"$@"}' && eval 'exec perl -S $0 $argv:q'
  if 0;

(the "if 0;" is on a separate line).  I imagine there are other uses of that in old code floating around which could be broken by our current shebang mangling.  Whether we would actively try to get rid of terrible crap like that in the distribution is a separate discussion.  It appears to be as bad as env for both security and reliability.

I don't want to live on this planet anymore.

This also works with empty shebang:

$ ./test
disgusting

$ cat test
#!
echo disgusting

(In reply to Jason Tibbitts from comment #23)
> I just wanted to note this _somewhere_: It is valid for an executable to
> have no shebang line; the kernel just passes it to /bin/sh.

That's not actually true:

strace /bin/mptopdf 
execve("/bin/mptopdf", ["/bin/mptopdf"], 0x7fff555ea3f0 /* 71 vars */) = -1 ENOEXEC (Exec format error)

The shell does it, and older versions of posix_spawn.  It's also what makes the empty shebang case work.

I need a "valid shebeng" definition. Until then, I cannot implement this. Is there a POSIX spec or something like that?

(In reply to Miro Hrončok from comment #27)
Looking for "valid shebang" definition is wrong. You should look at the problem in a way of "If I mangle the shebang, I know the result is correct". And if you don't recognize the format, the mangling should fail. This way would avoid broken shebangs.

I still don't know how to "know the result is correct".

All we have is syntax, existing policy and good sense.  What you can't know in the context of a brp_* script running at the end of the %install is something like whether the path is actually valid.

You know that if there is "#!/usr/bin/env python" shebang and you change it to "#!/usr/bin/python" that the result is correct. So change just this shebang and fail for others. If I later come and complain that you are failing, because there is "#!/usr/bin/env ruby" shebang, then add it to list of known shebangs to mangle.

Or in comment 11, I proposed the RedExp which should match shebangs we know, understand and believe they are correct. If there is some shebang which does not match this RegExp and the mangling script fails with it, then you can special case it, because I am pretty sure people will let you know.

Honestly if you claim that you don't know what is the "valid shebang", then mangling everything is quite bold IMO. It just means that the script corrupts shebangs and we have no chance to know that.

May be I should change the subject once more to "fail hard if the shebang format is not recognized".

^#![[:alnum:][:space:]./-]*$

What I also believe is totally ok in shebang:

Totally:
_
+
,


Theoretically:

€
💩
…

Do you have any examples in hand?

Real world examples? No. But I can make it work:

[shebangs]$ cat > 💩 << EOF
> #!/usr/bin/sh
> echo 💩💩💩
> EOF
[shebangs]$ cat > script << EOF
> #! /home/churchyard/tmp/shebangs/💩
> watch this
> EOF
[shebangs]$ chmod +x 💩
[shebangs]$ chmod +x  script 
[shebangs]$ ./script 
💩💩💩


Valid shebang.

note that I'm not so concerned about emoji in shebangs, but there are more normal characters (like _) that were not in the regex. Such regex is too strict.

maybe blacklist (instead of whitelisting) special characters for shell?

We can't fix this unless we have an exact definition of a valid shebang.
Please re-open if you know of one.

Sorry, but your resolution is contradicting IMO. Although you admit you don't know what the definition of valid shebang is, you don't mind changing all shebangs.

What I am saying is that if we don't know what the shebang definition is, we should be careful and mangle just shebangs we know we can safely mangle and error out for the rest, collecting more knowledge about shebangs and slowly relax the rules.

Honestly, I am fine if shebang like "#! /home/churchyard/tmp/shebangs/💩" is not handled ATM. The mangling can be disabled for this case or the algorithm might be gradually extended if this is proved to be widely used practice.

So the request in fact is: Don't mangle shebangs you don't understand?

(In reply to Miro Hrončok from comment #39)
> So the request in fact is: Don't mangle shebangs you don't understand?

Yes (or better "Mangle only shebangs you are sure you understand") and it has been since comment 11 or comment 16, when the original subject was updated when it was clear that the mangling script corrupts some shebangs.

In comment 21 you say:

> Fail hard and let packager to decide.

Is that still valid? So In fact you ask to:

"Mangle only shebangs you are sure you understand, fail hard on others"

Based on this we are sure rendering all packages with shebangs we don't understand FTBFS just because we don't understand something. Is that desired?

What is not desired is to ship packages with broken shebangs to our users. It is reasonable OK to fail a build if the output of mangling is uncertain.

Actually, if you did some review/analysis of shebangs after the last mass rebuild, e.g. how many shebangs were mangled and if the results are as expected, you might convince me that the mangling is reasonably ok. But I am afraid you don't have such numbers.

Working on it. Will get some numbers.

Here are some rough stats:  https://gist.github.com/hroncok/d153305d1386648b8ae5921953c7d0ec

Most interesting/weird things I see:

shebnags/nodejs-yarn-1.9.2-1.fc29.src.rpm.log:mangling shebang in /usr/lib/node_modules/yarn/node_modules/performance-now/test/scripts/difference.coffee from /usr/bin/env ./node_modules/.bin/coffee to #!/usr/bin/./node_modules/.bin/coffee
...


shebnags/contextkit-0.5.41-18.fc29.src.rpm.log:mangling shebang in /usr/bin/context-rlwrap from /usr/bin/env python2.5 to #!/usr/bin/python2.5

(In reply to Miro Hrončok from comment #44)
> Here are some rough stats: 
> https://gist.github.com/hroncok/d153305d1386648b8ae5921953c7d0ec

Not sure I understand how you get the output nor that I understand its content, but the two cases are indeed interesting. And also others such as:

~~~
" (('/usr/bin/env gpaw-python', '/usr/bin/gpaw-python'), 2),\n",
~~~

Because package with shebang like this has now broken dependency and is not installable because gpaw-python is not available on Fedora. That is presumably similar to the "python2.5" case.

The output is from all build logs of all packages in rawhide (latest successful build).

The broken dependency on gpaw-python thing just uncovers a bug in the package.
The file does not work without gpaw-python.

shebnags/gpaw-1.4.0-6.fc29.src.rpm.log:mangling shebang in /usr/bin/gpaw-runscript3 from /usr/bin/env gpaw-python to #!/usr/bin/gpaw-python
shebnags/gpaw-1.4.0-6.fc29.src.rpm.log:mangling shebang in /usr/bin/gpaw-runscript from /usr/bin/env gpaw-python to #!/usr/bin/gpaw-python

$ dnf repoquery --repo=rawhide --whatrequires /usr/bin/gpaw-python
python2-gpaw-0:1.4.0-6.fc29.x86_64
python3-gpaw-0:1.4.0-6.fc29.x86_64

$ dnf repoquery --repo=rawhide -f /usr/bin/gpaw-python
(nope)

I'll report that.

bz1639270

I'll go trough all the mangled shebnags and make sure we have those files in Fedora, file bugz otherwise.

From the list, the following shebangs are not listed/provided by any package:

/usr/bin/gpaw-python bz1639270
/usr/bin/./node_modules/.bin/coffee bz1674073
/usr/bin/perl6-j bz1674074
/bin/python (the script was fixed since not to do this) vdsm and fmtools will be fixed once rebuilt (they FTBFS)
/usr/bin/python2.5 bz1610014
/bin/ruby (the script was fixed since not to do this) no packages require this

We should fail hard in the case of relative path after /usr/bin/env (which gets mangled to /usr/bin/./…).
The other instances in Fedora are actual errors, uncovered by the mangling.

I have a delta here between F28 and F29 in regards to the dash shell. In F28, #!/bin/dash was left untouched, but in F29 it's mangled:
> mangling shebang in /usr/bin/testdash.sh from /bin/dash to #!/usr/bin/dash

[root@f29 rpmbuild]# dnf localinstall /home/rpmbuild/rpm/RPMS/x86_64/testdash-1-1.fc29.x86_64.rpm
Error: 
 Problem: conflicting requests
  - nothing provides /usr/bin/dash needed by testdash-1-1.fc29.x86_64
(try to add '--skip-broken' to skip uninstallable packages)


The dash package is identical between F28 and F29, but since DNF doesn't know how to translate /bin/dash to /usr/bin/dash, it fails on F29.

I feel like this belongs here, but I can make a new BZ as well.


[rpmbuild@f28 SPECS]$ rpm -q --requires /home/rpmbuild/rpm/RPMS/x86_64/testdash-1-1.fc28.x86_64.rpm
/bin/dash
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

[rpmbuild@f29 SPECS]$ rpm -q --requires /home/rpmbuild/rpm/RPMS/x86_64/testdash-1-1.fc29.x86_64.rpm
/usr/bin/dash
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1



[root@f28 x86_64]# rpm -q --provides dash
dash = 0.5.10.2-1.fc28
dash(x86-64) = 0.5.10.2-1.fc28
[root@f28 x86_64]# rpm -ql dash
/bin/dash
/usr/lib/.build-id
/usr/lib/.build-id/04
/usr/lib/.build-id/04/d1cfdf53fbe4ac4f8c24fc6cdda3f4be92332b
/usr/share/doc/dash
/usr/share/doc/dash/COPYING
/usr/share/doc/dash/ChangeLog
/usr/share/man/man1/dash.1.gz

[root@f29 rpmbuild]# rpm -q --provides dash
dash = 0.5.10.2-1.fc29
dash(x86-64) = 0.5.10.2-1.fc29
[root@f29 rpmbuild]# rpm -ql dash
/bin/dash
/usr/lib/.build-id
/usr/lib/.build-id/98
/usr/lib/.build-id/98/3fd81a08dcd44aba92345e39957bc711257995
/usr/share/doc/dash
/usr/share/doc/dash/COPYING
/usr/share/doc/dash/ChangeLog
/usr/share/man/man1/dash.1.gz

Please file a bugzilla for dash. They manually install to /bin instead of /usr/bin - they should not do that:

   %configure --bindir=/bin

Thanks.

(In reply to Miro Hrončok from comment #52)
> Please file a bugzilla for dash. They manually install to /bin instead of
> /usr/bin - they should not do that:
> 
>    %configure --bindir=/bin
> 
> Thanks.

Thanks. BZ #1691825

This message is a reminder that Fedora 28 is nearing its end of life.
On 2019-May-28 Fedora will stop maintaining and issuing updates for
Fedora 28. It is Fedora's policy to close all bug reports from releases
that are no longer maintained. At that time this bug will be closed as
EOL if it remains open with a Fedora 'version' of '28'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 28 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

This message is a reminder that Fedora 28 is nearing its end of life.
On 2019-May-28 Fedora will stop maintaining and issuing updates for
Fedora 28. It is Fedora's policy to close all bug reports from releases
that are no longer maintained. At that time this bug will be closed as
EOL if it remains open with a Fedora 'version' of '28'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 28 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

This message is a reminder that Fedora 29 is nearing its end of life.
Fedora will stop maintaining and issuing updates for Fedora 29 on 2019-11-26.
It is Fedora's policy to close all bug reports from releases that are no longer
maintained. At that time this bug will be closed as EOL if it remains open with a
Fedora 'version' of '29'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 29 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

I don't think this was resolved.

It wasn't however I am not sure what would be the best way to handle this.

Since we know no good definition of "invalid shebang", we can't fail hard for invalid shebangs.

This means the bug is not resolved, but also that at this point we can't do anything more to help fix it.
Please re-open if you disagree.