Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

urlscan should read from stderr #122

Closed
b0o opened this issue Feb 23, 2022 · 5 comments
Closed

urlscan should read from stderr #122

b0o opened this issue Feb 23, 2022 · 5 comments

Comments

@b0o
Copy link

b0o commented Feb 23, 2022

I use a shell script as my $BROWSER to launch my actual browser. My script writes to stderr; when invoked through urlscan, it seems stderr is not writable:

browser.sh:

#!/usr/bin/env bash
ls -la /proc/self/fd/ > /tmp/browser-out
$ BROWSER=/tmp/browser.sh urlscan urls.txt 
... open a URL and quit urlscan ...
$ cat /tmp/browser-out
total 0
dr-x------ 2 maddy maddy  0 Feb 23 03:02 .
dr-xr-xr-x 9 maddy maddy  0 Feb 23 03:02 ..
lrwx------ 1 maddy maddy 64 Feb 23 03:02 0 -> /dev/pts/222
lrwx------ 1 maddy maddy 64 Feb 23 03:02 1 -> /dev/pts/222
lr-x------ 1 maddy maddy 64 Feb 23 03:02 2 -> /home/maddy/bin/browser.sh
lr-x------ 1 maddy maddy 64 Feb 23 03:02 255 -> /home/maddy/bin/browser.sh

Writing to stderr returns a non-zero exit code, and since my script uses bash's -e (errexit) option, it crashes.

Reproduction:

$ cat browser.sh
#!/usr/bin/env bash
set -euo pipefail

echo 1 >/tmp/browser-out
echo stdout
echo 2 >>/tmp/browser-out
echo stderr >&2
echo 3 >>/tmp/browser-out
echo stdout
echo 4 >>/tmp/browser-out
echo stderr >&2
echo 5 >>/tmp/browser-out

$ BROWSER="$PWD/browser.sh" urlscan urls.txt
... open a URL and quit urlscan ...

If you look at /tmp/browser-out, the expected contents would be...

1
2
3
4
5

...but the actual contents are...

1
2

If I just don't write to stderr, everything works fine, but this seems like a bug and it took me a while to figure out what was going on. I think urlscan should either pipe stderr to the terminal or to /dev/null.

@b0o b0o changed the title Hangs if $BROWSER writes to stderr urlscan should read from stderr Feb 23, 2022
@firecat53
Copy link
Owner

In commit 01e4cfb I basically turned off stderr so the extraneous terminal (stderr) output from browsers or xdg-open would not disturb the urlscan display. This happens in urlscan/urlchoose.py (line 754):

savout = os.dup(2)
os.close(2)
os.open(os.devnull, os.O_RDWR)
....
os.dup2(savout, 2)
if thread is False:
    self.draw_screen()

I'm interested in keeping stderr suppressed from a usability standpoint. It's typically annoying to have to hit Ctrl-L every time after you open a URL.

I actually don't understand how your browser.sh script works! What functionality do you lose if you don't have stderr for this use case?

Thanks for your interest!

@b0o
Copy link
Author

b0o commented Feb 23, 2022

Thanks for the quick response. It seems like that code is trying to redirect stderr to /dev/null, am I correct? I’m not sure if it’s actually working because my script sees stderr as not writable at all.

The browser.sh scripts above are just for demonstration purposes. The first example I’m listing the contents of /proc/self/fd which shows that fd 2 (stderr) is not writable (it has permissions lr-x------).

The second example is just demonstrating that my script fails after the echo 2 >>/tmp/browser-out, which implies that it’s failing on the next line when attempting to write to stderr.

It’s not that I lose functionality, but that it was a rather unexpected failure and took a while to debug. I am fine with stderr going to /dev/null, but attempting to write to it should not fail. I imagine this could trip someone up in the future.

@firecat53
Copy link
Owner

Please give the develop branch a try. I changed how the redirection of
stdout/stderr works and it seems to work with your test case. Let me know if it
breaks anything else!

@b0o
Copy link
Author

b0o commented Mar 8, 2022

Fantastic, that works. Will let you know if anything breaks.

I also have to say that I love this tool and I appreciate your work on it. Thank you!

I'll let you decide if this should be closed now or when the change is merged into main.

@firecat53
Copy link
Owner

Thanks for the kind words!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants