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

Is access denied recoverable? #96

Closed
guybedford opened this issue Jan 17, 2024 · 15 comments
Closed

Is access denied recoverable? #96

guybedford opened this issue Jan 17, 2024 · 15 comments

Comments

@guybedford
Copy link
Contributor

guybedford commented Jan 17, 2024

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
Copy link
Collaborator

badeend commented Jan 17, 2024

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.

  • In pretty much all applications, bind and listen are called right after each other. I they hit any error, it's more likely they'll construct a whole new socket, rather than keeping the bound socket around and retrying the listen at a later moment.
  • For connect this is not supported by POSIX:

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
Copy link
Contributor Author

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
Copy link
Collaborator

badeend commented Jan 17, 2024

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
Copy link
Collaborator

badeend commented Jan 17, 2024

I added a generic remark to the documentation of Bind: e4febb3 without reference to any specific error codes.

WDYT?

@guybedford
Copy link
Contributor Author

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
Copy link
Contributor Author

(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
Copy link
Collaborator

badeend commented Jan 17, 2024

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: 99325cf

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

@guybedford
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

badeend commented Jan 18, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

badeend commented Jan 18, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

badeend commented Jan 18, 2024

(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
Copy link
Contributor Author

guybedford commented Jan 18, 2024

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.

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