shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

"cmd | grep --quiet" introduces race condition

Open gozdal opened this issue 7 years ago • 4 comments

  • [x] shellcheck.net (i.e. the latest commit) currently gives no useful warnings about this
  • [x] I searched through https://github.com/koalaman/shellcheck/issues and didn't find anything related

Here's a snippet or screenshot that shows the problem:

This script should be endless loop, but after some iterations it'll exit with an error (SIGPIPE delivered). grep --quiet closes input pipe when it matches pattern, while command on the left hand side will exit with an error trying to write to a closed pipe:

#!/bin/bash

set -euo pipefail

count=1

while echo -e "foo\\nbar\\nbaz\\n" | grep --quiet foo; do
    echo "$count"
    count=$((count+1))
done

Here's what shellcheck currently says:

Nothing

Here's what I wanted or expected to see:

Using "cmd | grep --quiet" with "set -o pipefail" introduces race condition, which may cause the script to fail. use "cmd | grep --count" if possible.

gozdal avatar Feb 07 '18 09:02 gozdal

Shellcheck doesn't run your code so if you turn pipefail off before you get to the grep it can't tell. AIUI it also scans the script from top to bottom so if you're using functions it may not have seen the pipefail yet.

Now for your simple example, without any flow control, it could work. But for consistency you would probably have to forbid pipefail being left on and so throw a "You seem to have left pipefail active on line N, this causes unexpected behaviour" if you get to a condition or other flow control (eg: return).

rdebath avatar Feb 14 '18 12:02 rdebath

This is a very interesting issue that ShellCheck should be warning about.

Yes, it's a mathematically undecidable problem, but that's easily solved through the magic of false positives :P

koalaman avatar Feb 25 '18 22:02 koalaman

Another fail scenarios: grep --max-count or head on the right hand side of a pipe.

gozdal avatar May 11 '18 14:05 gozdal

Similar issue is https://github.com/koalaman/shellcheck/issues/665

pothos avatar Jul 04 '22 12:07 pothos

I really like to see this check as I faced this issue, too: https://unix.stackexchange.com/questions/743260/has-grep-quiet-a-bug-with-its-exit-status

use "cmd | grep --count" if possible.

If cmd returns 1 million lines of text and the first line contains the match, this will be really slow compared to --quiet. Instead this could be a solution:

# treat SIGPIPE as true
cmd | grep --quiet foo || [[ $? -eq 141 ]]

Another possible solution is not to use pipefail at all or only selectively as mentioned here:

https://mywiki.wooledge.org/BashPitfalls#pipefail

If you know that each part of a pipeline beyond the first will consume all their input, pipefail is safe and often a good choice, but it should not be enabled globally, only enabled for pipelines where it makes sense, and disabled afterwards.

At the moment I tend to remove pipefail. Before I used it, I never had any problems and now I have to think about race conditions in pipes...

mgutt avatar Apr 18 '23 15:04 mgutt

I would suggest to close this as duplicate of https://github.com/koalaman/shellcheck/issues/665.

felipecrs avatar Aug 01 '23 20:08 felipecrs

FYI, I tend to always use … | { grep ARG || true ; } | … nowadays and the same for … | { cmd || true ; } | head. Suggesting this would be nice even without detecting that -o pipefail is set because it might be that this script gets sourced and the shell already has -o pipefail set.

pothos avatar Aug 01 '23 21:08 pothos

FYI, I tend to always use … | { grep ARG || true ; } | … nowadays and the same for … | { cmd || true ; } | head. Suggesting this would be nice even without detecting that -o pipefail is set because it might be that this script gets sourced and the shell already has -o pipefail set.

Covering all the exceptions caused by pipefail is much more than checking only the grep commands. For me there are only two options left:

  • never set pipefail
  • people sourcing my scripts with pipefail have to learn their lesson on their own

mgutt avatar Aug 02 '23 07:08 mgutt