WebAssembly / wasi-sockets

WASI API proposal for managing sockets
243 stars 22 forks source link

Is access denied recoverable? #96

Closed guybedford closed 9 months ago

guybedford commented 9 months ago

From #95: I implemented the permission denied errors as recoverable, in that one could imagine later granting the permission on the system and then trying the operation again on the socket. I'm open to whatever here, but I think it would be good to specify this behaviour.

Specifically should permissions errors transition back into the previous state before the *-in-progress state, or should they always move to the terminal closed state?

badeend commented 9 months ago

They should behave the same as any other non-would-block error. Meaning: for start/finish bind they should revert the state back to unbound. For start/finish listen&connect, it'll move to closed

one could imagine later granting the permission on the system and then trying the operation again on the socket.

I don't see this happening in practice.

If connect() fails, the state of the socket is unspecified. Conforming applications should close the file descriptor and create a new socket before attempting to reconnect. https://pubs.opengroup.org/onlinepubs/009695399/functions/connect.html

guybedford commented 9 months ago

Ok, this does mean that some access denied errors are recoverable and some are not, depending on which state they are thrown in, it might help to add some clarification on this noting it is permitted.

badeend commented 9 months ago

Doesn't this naturally follow from the state diagram?

I'd rather not add a clarification specifically for access-denied when that error code isn't relevant to the state transitions at all.

badeend commented 9 months ago

I added a generic remark to the documentation of Bind: https://github.com/WebAssembly/wasi-sockets/pull/98/commits/e4febb313a1cc657a8378eaa8c4b951d269dcc31 without reference to any specific error codes.

WDYT?

guybedford commented 9 months ago

Hmm, the question is more - can access-denied be thrown in both start-* and finish-* and if so, are both equally correct places to do it?

guybedford commented 9 months ago

(and if the answer is yes, we certainly don't need further clarification as it comes from the state machine as you say, but from an implementation perspective I'm just implementing a simple generic sync check in start-* for simplicity, and assuming thats ok)

badeend commented 9 months ago

can access-denied be thrown in both start- and finish- and if so, are both equally correct places to do it?

Ah, I see. Yes, both are equally correct. See update: https://github.com/WebAssembly/wasi-sockets/pull/98/commits/99325cff6762ebe06693e56b408b44b463895c97

In preview3+ this distinction between start and finish won't exist at all anymore.

guybedford commented 9 months ago

Thinking about this some more, due to the distinction between recoverability and non-recoverability I do think we need to state quite explicitly that invalid-argument etc are start errors, that are recoverable. But that access-denied can be both.

Alternatively we need to define recoverability for finish errors.

guybedford commented 9 months ago

So even when there is async, there is still the distinction between transitioning to the in-progress state, versus not, and errors can still fall on both sides of that.

badeend commented 9 months ago

due to the distinction between recoverability and non-recoverability

There is no distinction. Either something fails or it doesn't. The only special case is would-block

guybedford commented 9 months ago

There is no distinction. Either something fails or it doesn't. The only special case is would-block

There very much is a distinction - whether that failure transitions the socket into the closed state or retains the current state.

Are you saying that all invalid-state errors are terminal and non-recoverable and transition to the closed state then? This is certainly not what I have implemented so far, so that clarification would be a great help. And what about invalid-arguments? So far I've moved all invalid-arguments errors as much as possible into start- functions so that they are recoverable (and did at points in the implementation have them after the in-progress state transition).

I have found this can be done comprehensively for invalid-arguments since the finish- functions don't need to do those validations in all cases afaict.

Just trying to be sure I've implemented the right thing myself - nothing more, nothing less.

badeend commented 9 months ago

Just trying to be sure I've implemented the right thing myself - nothing more, nothing less.

I completely understand.

Are you saying that all invalid-state errors are terminal and non-recoverable and transition to the closed state then?

Fair point. No, those are indeed not terminal and they do not cause a state transition. This is described in the section "Not shown in the diagram":

Calling a method from the wrong state returns error(invalid-state) and does not affect the state of the socket. A special case are the finish- methods; those return error(not-in-progress) when the socket is not in the corresponding -in-progress state.

So a more correct statement would be: when a method is called from the correct state, there is no distinction between error codes (other than would-block).

And what about invalid-arguments?

Those are regular errors. So non-terminal for bind, terminal for connect & listen, etc.

guybedford commented 9 months ago

I think the only cases to clarify then are: (1) invalid-arguments does not perform a state transition, and is always thrown in start functions (contrary to the previous refactoring point about this not mattering), (2) access-denied can happen for start- or finish-, and this is the default as unspecified right now in being unconstrained, where the distinction between terminal and non-terminal is exactly as expected. From an implementation point of view, I'm very much implementing the non-terminal version even though I understand Wasmtime probably does the opposite. If my implementation should be considered non-standard in this regard, then certainly these ban it with spec text, but otherwise that answers my questions.

badeend commented 9 months ago

(1) invalid-arguments does not perform a state transition

Yes it can

invalid-argument (...) is always thrown in start functions

Very likely, but the spec doesn't require this.

guybedford commented 9 months ago

Treating the exact state transition behaviours in errors as implementation specific seems fine to me (ie whether the socket state transition occurs before or after throwing the error). The only risk I see here is implementations that rely on errors that retain the state in one implementation then not working in another. But I guess in reality creating a new socket is just the best way to handle that anyway so treating it as somewhat unspecified doesn't seem overly problematic.