Page MenuHomeFreeBSD

Mk/Scripts/qa.sh: Turn off pipefail when piping to grep -q
ClosedPublic

Authored by jrm on Jul 18 2021, 2:14 AM.

Details

Summary

The pipeline

readelf -d "${dep_file}" | grep -q SONAME

can fail because grep -q closes the output early resulting sigpipe being
sent to readelf. Other possible solutions are to turn off pipefail for
the file or remove the -q grep argument.

Credit to ashish@ for the detective work on discovering the root cause
of this.

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jrm requested review of this revision.Jul 18 2021, 2:14 AM

FWIW, I also came across it some time ago while working on www/qt5-webengine. The issue with grep -q can be reproduced quite well on i386 platforms with quite large files and I had already created a PoC for this, but since then I haven't had time to follow it up more closely.

Please add a comment about why it is disabled, otherwise it ends up being new magic that nobody knows why it is there.

Add a comment describing why pipefail is turned off when piping to grep -q.

foo | grep -q is a pretty common idiom so I worry that there could be many instances of this issue, not just readelf

@emaste, mat@ (via bsdports@efnet) is good starting with this fix for the known error. I'll take a look at the other instances of foo | grep -q in the future and perhaps try to find a more elegant fix at some point. Any objections?

In D31211#743859, @jrm wrote:

@emaste, mat@ (via bsdports@efnet) is good starting with this fix for the known error. I'll take a look at the other instances of foo | grep -q in the future and perhaps try to find a more elegant fix at some point. Any objections?

No objection to this change, I was just worried that there might be additional cases.

Interesting, I just found https://github.com/koalaman/shellcheck/issues/665 while looking at this.

Perhaps a better solution is to turn pipefail back off for the whole file. pipefail is an interesting concept, but might be problematic in practice. mat@, what do you think?

This revision is now accepted and ready to land.Nov 19 2021, 9:04 PM

Another option is to close stdin for the command feeding the pipe to grep (e.g., readelf):

readelf -d "${dep_file}" < /dev/null

Then we could continue to use pipefail to catch real errors from readelf and still use grep -q which is more efficient than grep > /dev/null.

Note there is another instance of readelf | grep -q in qa.sh - in sonames().

Maybe I'm misunderstanding you, but

if ! readelf -d "${dep_file}" < /dev/null | grep -q SONAME; then
   err "${file} is linked to ${dep_file} which does not have a SONAME.  ${dep_file_pkg} needs to be fixed."
fi

from proxydeps() in qa.sh has the same problem with or without /dev/null. You can confirm this with poudriere testport ... devel/bear.