dup() syscall is wrongly implemented

[socketpair]

dup()ed file descriptors should share all flags (and also seek position). Opening same file twice is not the same thing. Also, one may try to dup() socket, which cannot be open()ed.

[kripken]

Can you make a small standalone testcase for this? It would be useful to verify that we fix it properly, and then we'd put it in the test suite.

[socketpair]

I already done that. See tests :) I fetched output of dup.c using gcc and without emscripten.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past 2 years. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

[socketpair]

@kripken what is the actual status ?

[kripken]

@socketpair

I think I missed this issue back then because I didn't see the test, sorry about that! Do you want to open a PR with that? Looks like it hasn't been fixed since.

[socketpair]

Possibly. Unfortunatelly not right now.

[jakogut]

@socketpair @kripken I've rebased this on incoming, but the test no longer passes. The seek position and flags work properly, but the duplicated file descriptors don't close properly.

Looking into the issue, it appears that doDup in library_syscall.js assigns the reference to the stream to be duplicated to the suggested fd. This makes flags and seek positions work as expected, but upon closing either the original fd or the duplicated fd, the shared reference has its fd set to null, and it can no longer be closed on a duplicate stream.

Unless I'm mistaken, it seems that stream parameters such as flags and seek position are shared between duplicated streams, but the closed status is not?

[jakogut]

I'm no JS guru, but it looks to me like we might want a new prototype for shared stream attributes.

[jakogut]

Well, I have a branch (https://github.com/jakogut/emscripten/tree/fix-dup-syscalls) that works with manual testing like so:

emcc tests/unistd/dup.c -o dup.js && node dup.js

Output:

DUP
errno: 0
f: 1
f2,f3: 1
1. f3 offset was 0.    Should be 0
2. f  offset set to 1. Should be 1
3. f2 offset set to 2. Should be 2
4. f  offset now is 1. Should be 1
5. f2 offset now is 2. Should be 2
6. f3 offset now is 1. Should be 1 (and not 0)
7. f3 offset set to 3. Should be 3
8. f  offset now is 3. Should be 3 (and not 1)
close(f1): 0
close(f2): 0
close(f3): 0

DUP2
errno: 0
f: 1
f2,f3: 1
close(f1): 0
close(f2): 0
close(f3): -1

Strangely, using the test runner, it still fails like it's unpatched.

python2 tests/runner.py wasm0.test_unistd_dup

Output:

Test suites:
['test_core']
Running test_core: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_unistd_dup (test_core.wasm0) ... (test did not pass in JS engine: ['/usr/bin/node'])
FAIL

======================================================================
FAIL: test_unistd_dup (test_core.wasm0)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/joseph/emscripten/tests/test_core.py", line 111, in decorated
    func(self, js_engines=[NODE_JS])
  File "/home/joseph/emscripten/tests/test_core.py", line 5122, in test_unistd_dup
    self.do_run(src, expected, js_engines=js_engines)
  File "/home/joseph/emscripten/tests/runner.py", line 1140, in do_run
    self.assertContained(expected_output, js_output, check_all=assert_all)
  File "/home/joseph/emscripten/tests/runner.py", line 843, in assertContained
    additional_info
AssertionError: Expected to find 'DUP
errno: 0
f: 1
f2,f3: 1
1. f3 offset was 0.    Should be 0
2. f  offset set to 1. Should be 1
3. f2 offset set to 2. Should be 2
4. f  offset now is 1. Should be 1
5. f2 offset now is 2. Should be 2
6. f3 offset now is 1. Should be 1 (and not 0)
7. f3 offset set to 3. Should be 3
8. f  offset now is 3. Should be 3 (and not 1)
close(f1): 0
close(f2): 0
close(f3): 0

DUP2
errno: 0
f: 1
f2,f3: 1
close(f1): 0
close(f2): 0
close(f3): -1
' in 'DUP
errno: 9
f: 1
f2,f3: 1
1. f3 offset was 0.    Should be 0
2. f  offset set to 1. Should be 1
3. f2 offset set to 2. Should be 2
4. f  offset now is 1. Should be 1
5. f2 offset now is 2. Should be 2
6. f3 offset now is 0. Should be 1 (and not 0)
7. f3 offset set to 3. Should be 3
8. f  offset now is 1. Should be 3 (and not 1)
close(f1): 0
close(f2): 0
close(f3): -1

DUP2
errno: 0
f: 1
f2,f3: 1
close(f1): 0
close(f2): -1
close(f3): -1
', diff:

--- expected
+++ actual
@@ -1,5 +1,5 @@
 DUP
-errno: 0
+errno: 9
 f: 1
 f2,f3: 1
 1. f3 offset was 0.    Should be 0
@@ -7,18 +7,18 @@
 3. f2 offset set to 2. Should be 2
 4. f  offset now is 1. Should be 1
 5. f2 offset now is 2. Should be 2
-6. f3 offset now is 1. Should be 1 (and not 0)
+6. f3 offset now is 0. Should be 1 (and not 0)
 7. f3 offset set to 3. Should be 3
-8. f  offset now is 3. Should be 3 (and not 1)
+8. f  offset now is 1. Should be 3 (and not 1)
 close(f1): 0
 close(f2): 0
-close(f3): 0
+close(f3): -1

 DUP2
 errno: 0
 f: 1
 f2,f3: 1
 close(f1): 0
-close(f2): 0
+close(f2): -1
 close(f3): -1




----------------------------------------------------------------------
Ran 1 test in 1.205s

FAILED (failures=1)

I've created a PR for review here: #9396

[socketpair]

According to posix, everything should be shared, except o_cloexec flag. Close() must remove the fd from the process namespace and then possibly destroy underlying object t. If fd is a socket, it should close connection (i.e. call shutdown() internally) only if latest reference is closed. Calling shutdown() must influence all fd duplicates, but leave them "open" (and unusable)

The same about closing pipe fds.

In short, real object destruction should happen only when closed last fd referring it.

In multiprocess environment, duplicates of descriptors might present in different processes.

Note, opening the same file must give another object which must not share state with one opened previously.

Also some picky details about flock() syscall

[socketpair]

If fd is duplicated and then closed, original fd must be accessible in all ways as if duplication and closing did not happen.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

[socketpair]

It was actual at time of writing. Don't know if still actual.

[trybka]

I just ran into this issue as well. Any chance of landing a fix here?

[hoodmane]

I opened another issue #15012 which may well also be caused by this.

[kripken]

Checking the testcase in jakogut@cfacd7d (copied and slightly modified here to avoid the need for JS, https://gist.github.com/kripken/381aa555c2597bd5ebb28c83b2cf0add), looks like it passes in the new WASMFS work (#15041).

@ethanalee @tlively I might be missing something, but it doesn't look like we have a test for this atm - dup tests don't use seek, and vice versa. If so, it might be good to add a test for this issue.

[ethanalee]

Thanks for the gist. This looks like a good test case to add. Thanks will make a note for the following PR!

[kripken]

Note that while this issue was closed, this only works in the new FS, which is not yet on by default. Perhaps we should reopen this issue, but then we might forget to close it later (and hopefully not much later in the future), so leaving closed.

[socketpair]

@kripken I did not check the change. Dup and dup2 should not share O_CLOEXEC flag. It's per FD. And also the flag should be honored in execve() - automatically close FDs where the flag is set. Should not be closed on fork().

[sbc100]

Re-opening since we are still have a broken dup as of today. We can close it if/when the FS becomes the default or if someone wants to fix the current FS implementation.

[tlively]

@socketpair, Emscripten does not have execve or fork, so I think O_CLOEXEC is a no-op anyway.

[socketpair]

Hmm. I think execve is possible to implement. Fork - no, but execve yes.

[sbc100]

emscripten is fundamentally single process (at least today) so there is no exec or fork or anything like that.

[socketpair]

@sbc100 yes, I know. Concept of execve does not mean creating new processes. Execve means "convert" working program to another. Without any parallelism.

[sbc100]

OK, but that is something else that we also have no support for today.

[hoodmane]

I think I fixed this in #17184. Anyways, the test case added in #9396 passes.