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

#3726 vauth: return CURLE_OUT_OF_MEMORY only in case of out of memory #3848

Closed
wants to merge 1 commit into from

Conversation

@dhoelzl
Copy link
Contributor

commented May 6, 2019

#3726 vauth: return CURLE_OUT_OF_MEMORY only in case of out of memory; return CURLE_RECV_ERROR in case of other errors

Dominik Hölzl
#3726 vauth: return CURLE_OUT_OF_MEMORY only in case of out of memory…
…; return CURLE_RECV_ERROR in case of other errors

@dhoelzl dhoelzl force-pushed the dhoelzl:master branch from 4a58c63 to 700e052 May 6, 2019

@kdudka

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

CURLE_OUT_OF_MEMORY indeed does not make any sense on certain places in the code. On the other hand, CURLE_RECV_ERROR is not always precise either. Would not CURLE_LOGIN_DENIED be closer to the nature of the failures?

@bagder
bagder approved these changes May 6, 2019
@dhoelzl

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

You are correct. I have not checked where to use CURLE_LOGIN_DENIED instead of CURLE_RECV_ERROR.
What about CURLE_SEND_ERROR in case of processing a response succeeds, but generating the next request fails?
@kdudka could you please check this? Thank you.

@kdudka

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

The problem is that the failure might not be network-related at all. I think that the most common case, which #3726 is about, is just missing credential cache and/or missing Kerberos configuration, which are both local failure.

@dhoelzl

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

There are a lot of return codes from gssapi and sspi. Maybe there is a need for a mapping of those error codes to cURL error codes?

@jay

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Maybe it would be better to have a generic CURLE_AUTH_ERROR or something? We seem to have a lot of these and recv error is not entirely correct, though I'm also of the opinion that it's better than an out of memory error.

@jay jay added the Authentication label May 9, 2019

@kdudka

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

@jay I agree. libcurl currently does not have any suitable error code for authentication failures triggered by local environment.

@dhoelzl dhoelzl closed this May 9, 2019

@dhoelzl

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I close this PR, as the based issue has been resolved, and I currently have not the time to investigate deeper into the different error situations / error codes to distinct between CURLE_LOGIN_DENIED/CURLE_RECV_ERROR/CURLE_SEND_ERROR/CURLE_AUTH_ERROR etc. as all the cases probably need to be tested in detail on all platforms.

jay added a commit to jay/curl that referenced this pull request May 11, 2019
vauth: Use CURLE_AUTH_ERROR for auth function errors
- Add new error code CURLE_AUTH_ERROR.

Prior to this change auth function errors were signaled by
CURLE_OUT_OF_MEMORY and CURLE_RECV_ERROR, and neither one was
technically correct.

Ref: curl#3848

Co-authored-by: Dominik Hölzl

Closes #xxxx

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.