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.

There is still a tidy warning left to ponder on: the left shift at the end of int2str() in x509asn1.c is done on a signed variable...

Do I push a fix on master or can I push to branch origin/bagder/clang-tidy ?

the goal of all these success/failure tests is only to silent the tidy warnings, but will grow the code footstep uselessly.

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 void to explicitly not return success/error for those places in the code where we currently skip the check.

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).

I'm not anymore in a position to test all changes to x509asn1.c

We really should create unit tests for this function so that it'd be tested by the CI.

Do I push a fix on master or can I push to branch origin/bagder/clang-tidy ?

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.

The question is: can we be really sure of that?

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.

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 void to explicitly not return success/error for those places in the code where we currently skip the check.

Alternatively, we could use an exception-like mechanism based on longjmp(), but this function is not yet used in libcurl. This would save a lot of tests at the cost of some additional storage for each handle, limit the footstep growth, and could even be used elsewhere in libcurl. If you are interested in this solution, I can develop macros for it.

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.

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.

Alternatively, we could use an exception-like mechanism based on longjmp

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.

Here I'm introducing test case 1651 that verifies the Curl_extract_certinfo() function (if the function is built, which depends on what TLS backend that's used) - and with only a very basic "fuzzing" (writing byte 1 to a few different indexes) I could immediately trigger an infinite loop in the original code but the version in this branch "succeeds" (by succeeding I mean that it doesn't get stuck).

Here I'm introducing test case 1651 ... I could immediately trigger an infinite loop in the original code ...

This test calls Curl_extract_certinfo() without a previous validation by the SSL library, which is the exact thing that should not be done: certificates passed to x509asn1.c procedures are supposed to be pre-validated by the SSL library. Again, see comment at x509asn1.c:99.

I'll let you decide if you want to introduce these checks anyway.

I'll push a fix for the signed left shift soon.

certificates passed to x509asn1.c procedures are supposed to be pre-validated by the SSL library

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:

1. We don't know exactly what level of pre-validation that is done by these different SSL libraries and I'm convinced that they all don't validate the entire certificate and all its fields. The question is then which fields they left unvalidated for us. I assume the exact set will differ depending on SSL library.
2. Assuming correct input is a trap that makes these functions almost impossible to test/fuzz which then also makes it very hard for us to discovered "normal" bugs on valid input. We don't know exactly what "validated" means, as we have different parsers and thus different levels of strictness and different error handling logic.

In other words: I think we must make these functions able to survive crap/malicious input.