Page MenuHomeFreeBSD

Mk/Scripts/qa.sh: badlibmix -- Checks that elfs aren't linked to both base and port SSL libraries and same with libgcc_s.so and gfortran
Needs ReviewPublic

Authored by yuri_rawbw.com on Aug 1 2017, 9:09 PM.

Details

Reviewers
None
Group Reviewers
portmgr
Summary

qa stage badlibmix that reports when elfs are linked to both base and port openssl, or both base and port libgcc_s.so. Such mixes often cause crashes, and shouldn't happen.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=221134

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

yuri_rawbw.com created this revision.Aug 1 2017, 9:09 PM
mat added inline comments.Aug 1 2017, 9:21 PM
Mk/Scripts/qa.sh
849–861

All these should probably be replaced by things like:

case "${dep_file}"
  (/usr/lib/libssl|/lib/libcrypto).so)
    ​ base_ssl_libs="${base_ssl_libs} ${dep_file}"
esac

saving us many forks

863

You should probably run ldd -a and only use the first block.

864–865

useless use of grep

awk '/=>/ {print $3}'
869
  1. never, ever, use `, always use $()
  2. I am not sure I see the point of using echo to add a string to a string.

This should also work.

"${file} is linked to both base and port SSL libraries: ${base_ssl_libs} and ${port_ssl_libs}"
yuri_rawbw.com added inline comments.Aug 1 2017, 9:32 PM
Mk/Scripts/qa.sh
849–861

Ok, will modify the patch.

863

No, lack of -a is by design. You want to check the whole dependency set, not only an immediate subset.

869
  1. Ok, will modify the patch.
  2. echo ${str} strips the leading space. Otherwise, this space should be taken out of the main echo string which will make it less readable.
yuri_rawbw.com added inline comments.Aug 1 2017, 9:33 PM
Mk/Scripts/qa.sh
863

No, the lack of -a is by design. You want to check the whole dependency set, not only the immediate subset.

yuri_rawbw.com added inline comments.Aug 1 2017, 9:49 PM
Mk/Scripts/qa.sh
864–865

It removes the first line which is an elf name.

Updated according to Mat's suggestions.

yuri_rawbw.com added inline comments.Aug 1 2017, 10:03 PM
Mk/Scripts/qa.sh
869

FYI: The same file contains backquotes in several other places.

yuri_rawbw.com marked 9 inline comments as done.Aug 1 2017, 10:04 PM
mat added inline comments.Aug 2 2017, 9:05 AM
Mk/Scripts/qa.sh
864–865

grep 'foo' | awk '{bar}' is equivalent to awk '/foo/ {bar}'. Please remove the grep.

869

If a single space throws you off, then change the code up there to, for example:

port_ssl_libs="${port_ssl_libs:+${port_ssl_libs} }${dep_file}"

or, if it does not work, the more ugly:

port_ssl_libs="${port_ssl_libs}${port_ssl_libs:+ }${dep_file}"
869

Yes, and because I have not had time to fix them does not mean you should use them :-)

Updated according to Mat's suggestions.

yuri_rawbw.com marked 3 inline comments as done.Aug 2 2017, 9:36 AM
mat added inline comments.Aug 2 2017, 10:06 AM
Mk/Scripts/qa.sh
853

Unless you are building openssl and friends, you will not have a PREFIX/lib/libssl.so and such, so you can remove the ones with PREFIX here and below for libgcc_s.

874

The message is slightly wrong, it may not really be linked with both, as we are checking recursive linking. But I am not sure how it could be changed.

yuri_rawbw.com added inline comments.Aug 2 2017, 10:32 AM
Mk/Scripts/qa.sh
853

Isn't it better to be general and always include both no matter what?

874

Even only recursive presence of both base and port libgcc_s.so is bad, because libgcc_s.so contains exception processing. libgcc_s.so is primarily, if not solely used for exceptions in various languages. Exception objects tend to go through libraries in arbitrary order. They can't be touched by procedures from different versions of libgcc_s.so. This causes crashes because different libgcc_s.so assume differently structured exception objects.

For the purposes of this check it's right to be conservative, and only consider the whole set of linked libraries.

yuri_rawbw.com marked 4 inline comments as done.Aug 2 2017, 10:33 AM
mat added inline comments.Aug 2 2017, 10:36 AM
Mk/Scripts/qa.sh
853

It is better to not do things that make no sense, in this case, there will never be a PREFIX/lib/libssl.so, so don't add it.

yuri_rawbw.com marked an inline comment as done.Aug 2 2017, 10:45 AM
yuri_rawbw.com added inline comments.Aug 2 2017, 5:24 PM
Mk/Scripts/qa.sh
869

Out of curiosity, why do you oppose backticks? They are a valid Bourne shell syntax feature. Do you find them aesthetically unappealing? Making code harder to read? Easy to confuse with regular quotes?

mat added inline comments.Aug 2 2017, 5:36 PM
Mk/Scripts/qa.sh
869

See Use $(STATEMENT) instead of legacy STATEMENT:

Backtick command substitution is legacy syntax with several issues.

  1. It has a series of undefined behaviors related to quoting in POSIX.
  2. It imposes a custom escaping mode with surprising results.
  3. It's exceptionally hard to nest.
yuri_rawbw.com marked 2 inline comments as done.Aug 2 2017, 5:38 PM
yuri_rawbw.com added inline comments.
Mk/Scripts/qa.sh
869

Thanks!

yuri_rawbw.com marked 2 inline comments as done.Aug 2 2017, 5:38 PM
db added a subscriber: db.Nov 6 2017, 3:25 PM

I don't know about the libssl case but I can tell you this does not take care of the case where a module referencing a libgcc lib is loaded later. See https://wiki.freebsd.org/libgcc%20problem