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
clang-tidy target and fixes #3163
Conversation
I initially omited these success/failure checks on purpose, because the parsed certificate format has already been validated by some SSL backend and these procedures are not intended to be used for certificate format validation. So these new checks should never fail... unless a code bug! As comment at x509asn1.c:98 says, there is no intention here in rewriting an SSL library, thus I kept it as small and simple as possible. As a consequence, the goal of all these success/failure tests is only to silent the tidy warnings, but will grow the code footstep uselessly. As you know, I'm not anymore in a position to test all changes to x509asn1.c, but they seem OK to me after an eye check.
Do I push a fix on master or can I push to branch origin/bagder/clang-tidy ? |
The question is: can we be really sure of that? If we really can be sure of that, then I'd probably instead suggest that we provide a macro or second function that explicitly uses Ultimately, I want to enable 'clang-tidy' runs in the CI and elsewhere so we need to fix the warnings, and this appears to be a warning that I'd rather not just disable globally (which I've done with a few others that just don't work out for us, as can be seen in this PR).
We really should create unit tests for this function so that it'd be tested by the CI.
Until we've decided on how to handle the return codes I think it is easier and has a lesser conflict-risk if you fix that independently of this branch. I think I'll merge the gtls.c fix independently too... |
For the record: the list of clang-tidy warnings my clang-tidy-8 shows right now on git master. |
At least, this is the intention: currently, this code is only called after a successful parse from an SSL library. Failures can only occur on a parse error (there's no allocation in this code), and could only be explained by a bug, either locally or in the SSL library.
Alternatively, we could use an exception-like mechanism based on |
If there's a bug in this code, a malicious HTTPS server that knows the client is curl could possibly craft a response to trigger that particular bug. The SSL library that handles the TLS handshake and everything is not going to police and correctness-check the entire thing before we get a chance to parse the cert. So there's a risk that there's "crafted crap" coming in here and it is important that our parser can handle almost any junk passed in without doing anything bad.
I'm not a fan of such custom solutions. They make the code look unusual and is harder for random developers to read and understand. I much prefer regular error checks plus returns. |
On 26 Oct 2018, at 12:44, Daniel Stenberg ***@***.***> wrote:
I'm not a fan of such custom solutions. They make the code look unusual and is harder for random developers to read and understand. I much prefer regular error checks plus returns.
+1. The explicit checks also makes the intent very clear.
|
The custom version used for Gskit, NSS, GnuTLS, WolfSSL and schannel. Currently hangs!
346dd5e
to
a691304
Compare
Here I'm introducing test case 1651 that verifies the |
This test calls I'll let you decide if you want to introduce these checks anyway. I'll push a fix for the signed left shift soon. |
I know it says so and I know that has been the assumption so far. I'm suggesting that we can and should do better:
In other words: I think we must make these functions able to survive crap/malicious input. |
Use an unsigned variable: as the signed operation behavior is undefined, this change silents clang-tidy about it. Ref: #3163 Reported-By: Daniel Stenberg
This adds a 'tidy' make target in the lib directory and two associated cleanup commits that address tidy warnings.
There is still a tidy warning left for @monnerat to ponder on: the left shift at the end of int2str() in x509asn1.c is done on a signed variable, and this isn't a defined behavior so we should either make it unsigned or not use shift...
Also @monnerat, I added lots of return code checks to x509asn1.c in this PR that fixes tidy warnings but I could certainly use an extra set of eyes to check that I did mess those up too bad!