alias asynchronous execution (&) seems broken

[avih]

Normal alias works, a-sync alias seems broken:

$ unset -f x
$ alias x='sleep 2'
$ x  # OK: prompt returns after 2s
$ x&  # BAD: prompt returns after 2s - should return immediately
$ x &  # BAD: doesn't run
sh: syntax error: unexpected "&"

For comparison, commands or functions or alias with additional arguments work OK:

$ sleep 2  # OK: prompt returns after 2s
$ sleep 2&  # OK: prompt returns immediately, later ENTER shows that the process ended
$ sleep 2 &  # OK: prompt returns immediately, later ENTER shows that the process ended

$ unalias x
$ function x() { sleep 2; }
$ x  # OK: prompt returns after 2s
$ x&  # OK: prompt returns immediately, later ENTER shows that the process ended
$ x &  # OK: prompt returns immediately, later ENTER shows that the process ended

$ unset -f x
$ alias x=sleep
$ x 2  # OK: prompt returns after 2s
$ x 2&  # OK: prompt returns immediately, later ENTER shows that the process ended
$ x 2 &  # OK: prompt returns immediately, later ENTER shows that the process ended

(Edited the examples and order)

[rmyorston]

Here's what I've found:

• There's only one issue: x & works fine, the failure seen above is a side effect of the previous x& failing.
• The issue is also present in upstream BusyBox (since 2016) but not in dash.

I've applied a fix which doesn't seem to cause any other issues, but this is tricky stuff and I welcome any testing. I've prepared a patch for upstream; I won't submit it until I have more confidence that it works.

As usual, prereleases (PRE-5339 or above).

[avih]

• There's only one issue: x & works fine, the failure seen above is a side effect of the previous x& failing.

Can confirm. My bad, sorry, and thanks for the investigation into what is an upstream issue (which I suspected it might be).

I've prepared a patch for upstream

I presume the upstream patch is moving that block from preadbuffer to __pgetc, just like this patch does when ENABLE_PLATFORM_MINGW32 is true, except unconditionally?

this is tricky stuff and I welcome any testing

I'll update my daily driver busybox binary to include this patch, but I'm not really sure what "testing" means here, and I don't use this form frequently (async alias, or even async in general).

So I'll report any possibly weird issues, but if you could list some use cases which you think get near this change, then I'll try to pay more attention.

[rmyorston]

Yes, the upstream patch is just the same but unconditional. If it's accepted upstream the conditionality can be removed here.

It's difficult to know what might be affected by this change. One of my earlier attempts managed to break here documents. So, just look out for anything weird.

[avih]

K, I'll pay more attention in general.

I presume the test results are the same?

Might also be worth adding a test case for async alias, and maybe also for the here-doc case which broke temporarily?

[rmyorston]

Despite what the README in the prerelease directory says, prerelease binaries are tested, just not the full test matrix.

Thinking about this some more:

• The code is similarly broken in dash, but it doesn't seem to matter there. At least, I can't find any way to trigger the problem.
• There's a second failure case in BusyBox: process substitution also requires a two character look-ahead. So x< didn't work properly before the recent fix.

Previously:

~ $ alias x='echo yo'
~ $ x<
yo
~ $ 
sh: syntax error: unexpected newline
~ $

Now:

~ $ alias x='echo yo'
~ $ x<
sh: syntax error: unexpected newline
~ $

[avih]

I didn't see what the readme says, and I wasn't aware there's more than just make test, but since you mentioned breakage at some stage, I was wondering and presuming that the tests don't show anything unexpected.

Anyway, I've been using a new build with your fix, and so far haven't noticed anything weird.

I think it might be worth sending what you have to the list or bugzilla, mentioning the known caveats, and hopefully others would be able to help.

Though it does seem that the volume at the ML is rather low, and so is upstream activity in general in recent months, which is a bit worrying...

[rmyorston]

I think it might be worth sending what you have to the list

Yes, I think I might as well. I've done as much as I can myself.

The other thought was to take it to the dash mailing list. The maintainer there has been active recently, which doesn't happen all that often. But they've submitted a patch set to support multi-byte characters which reworks how read-ahead is handled. If I understand it correctly this should do away with the problem reported here.

Though it does seem that the volume at the ML is rather low, and so is upstream activity in general in recent months, which is a bit worrying...

Yes, the upstream maintainer probably has more pressing issues.

[avih]

The other thought was to take it to the dash mailing list

Well, I'd think additionally rather than instead of. I'd think we do wants it on record for bb as well.

... they've submitted a patch set to support multi-byte characters which reworks how read-ahead is handled.. If I understand it correctly this should do away with the problem reported here.

Not sure I follow. You consider reporting it to the dash ML and at the same time you think it doesn't have this issue?

Or maybe it's a pending patch at the ML which didn't make it into the repo yet?

FWIW, I tested dash master, and this works correctly, unlike busybox:

$ alias x='sleep 2'
$ x&  # returns immediately

(commit 509f5b0 redir: Use memfd_create instead of pipe 2024-04-28 Herbert Xu)

Which I guess is in line with

The code is similarly broken in dash, but it doesn't seem to matter there. At least, I can't find any way to trigger the problem.

[avih]

FWIW, I tested dash master, and this works correctly, unlike busybox:

$ alias x='sleep 2'
$ x&  # returns immediately

This also works OK with FreeBSD 12.2 sh, and NetBSD 9.0 sh - both few years old, and also both decendants of the original ash and similar in features and behavior to dash and busybox ash, and also frequently share fixes with dash.

(built using a private portability setup to build them on non-BSD systems, like linux and cygwin and illumos and osx, which I wanted to make public, but haven't... yet)

[rmyorston]

There's an underlying problem with the input code. It's present in BusyBox ash, dash and (at a cursory inspection) FreeBSD sh.

The issue reported here is triggered by BusyBox's support for certain bash features. If BusyBox is built with CONFIG_ASH_BASH_COMPAT disabled the issue goes away.

[rmyorston]

Actually, at a slightly less cursory inspection, FreeBSD sh has different code in that area.

[avih]

I guess we're left with at least the BB report as a starting point.

I see you reported to the BB ML only the x< issue, but not the x& issue (and didn't mention it gets fixed as well by the patch).

What is x< supposed to do? I don't think I identify this form at the bash manpage at the REDIRECTION section...

[rmyorston]

I see you reported to the BB ML only the x< issue, but not the x& issue

They have the same underlying cause and x< was easier to explain. It's obvious that the echo is executed, whereas with x& it's necessary to explain that the process runs in the foreground.

What is x< supposed to do?

It doesn't do anything useful. It should fail. But in BusyBox ash it fails wrongly.

[rmyorston]

A more realistic example of failure with < is:

~ $ alias x=cat
~ $ x</dev/null
sh: can't open dev/null: no such file
~ $

[rmyorston]

The fix has been accepted by upstream BusyBox and busybox-w32 has been synced with upstream. Divergence has been reduced!

[avih]

I noticed both. Thanks!

[rmyorston]

This issue should be resolved in the latest release, FRP-5398, and has also been fixed upstream.