landley / toybox

toybox
http://landley.net/toybox
BSD Zero Clause License
2.42k stars 335 forks source link

grep hangs indefinitely on FIFO files #523

Open firasuke opened 4 days ago

firasuke commented 4 days ago

Hey there,

Thank you for your time and effort on toybox!

I came across the following behavior when using toybox grep. It appears to freeze or stall indefinitely when the file is a FIFO (named pipe).

To reproduce this issue, simply create a fifo file in a directory:

mkfifo -m 600 namedPipe

Then attempt to run toybox grep -rnw . -e "some expression" (in the directory where the namedPipe exists in or in one of its subdirectories) and it will freeze indefinitely.

Please note that both GNU grep and ugrep do not suffer from this issue (it is possible that they are skipping these special files entirely by default?).

Thanks in advance!

landley commented 1 day ago

On 10/17/24 15:03, Firas Khalil Khana wrote:

toybox grep appears to freeze or stall indefinitely when the file is a FIFO (named pipe).

Hmmm, according to strace, debian's grep is opening the file O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW. I noticed the O_NOFOLLOW because it breaks grepping a cp -s tree of symlinks, and I don't intend to copy that. (The -r vs -R logic is already distinguishing DIRECTORY symlinks, file symlinks should really act like the file they point to and refusing to do that is breakage unless Posix specified something weird recently that I missed?)

O_RDONLY we're already doing (it's 0, strace is hallucinating it here because neither O_RDWR nor O_WRONLY were specified, don't ask me what happens if you OR those together...)

O_NOCTTY seems fine, my question with O_NONBLOCK is what happens if you're reading from a network filesystem that takes a bit to talk to the other end, or you're on an external spinny media disk that needs to spin up? How much block counts as block? (I'm aware it's really easy for NFS to get stuck in D state, maybe this is why?)

shrug For now I can add the two new flags and a test, but heads up for Elliott that this might cause weirdness out in the giant surface area of android devices? (Probably won't, but...)

Commit 4073e77933d2