rustls / rustls-ffi

Use Rustls from any language
Other
124 stars 31 forks source link

Various small test fixes, initial clang-tidy linting coverage. #326

Closed cpu closed 1 year ago

cpu commented 1 year ago

Description

I generally have what I think is a healthy fear of writing C code and so while working in this codebase I've been looking for any/all linters and guard-rails I can deploy to keep the code I write safe. Along that line I've been seeing a bunch of warnings in the test code in CLion that are sourced from clang-tidy.

For my own sanity and understanding here's an attempt at fixing most of them, and landing a simple .clang-tidy config and CI step to prevent backsliding.

I've explicitly not fixed:

I've also (at least temporarily) disabled clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling - this is complaining about the use of memset and snprintf but I believe the suggested alternatives (memset_s and snprintf_s) require c11 and when I add --stdc11 to the CFLAGS I see a bunch of other errors and I ran out of time for chasing these today. Probably good follow-up work?

I recommend reviewing commit-by-commit. I'm not a confident C programmer so please call me out if I've made any incorrect assumptions in my fixes.

cpu commented 1 year ago

I'm going to merge this with ctz's +1 since it's test-only content. If any of these changes turn out to be controversial I'll commit to follow-up work to address any feedback that arrives post-merge, or to revert as required.

jsha commented 1 year ago

These changes all look great. Thanks for adding additional linting and for making the fixes.

initializer values not being used

I think most of these could be fixed by moving the initialization down to the site of the first use. I originally wrote this code with an eye towards the C89 standard (which requires declarations at the top of the function). Curl still targets C89 in order to get the widest portability possible, and I wanted to get used to the style before contributing to curl. However, it looks like I never added -std=gnu89 or even the more reasonable -std=gnu99 to the Makefile. Not sure why!

It looks like the current code compiles with -std=gnu99 on clang, but not with -std=gnu89. In rustls.h, cbindgen produces bool variables, which are C99 only. And commas at the ends of enumerator lists.

Anyway long story short I think we should probably target "C99 with GNU extensions" for the tests, and given that it's fine to move declarations down to their place of first use, which mostly dodges the question of whether to initialize variables ahead of use.

Now I'm curious why curl is okay compiling against rustls with bool and trailing commas in rustls.h... 🤔

"condition is always false when reached" ... EWOULDBLOCK being defined as equal to EAGAIN in errno.h on my machine

👍🏻

Googling, and via Stack Overflow, I see that GNU docs say:

To make your program portable, you should check for both codes and treat them the same.

cpu commented 1 year ago

Thanks for digging in to those couple items :-)

Anyway long story short I think we should probably target "C99 with GNU extensions" for the tests, and given that it's fine to move declarations down to their place of first use, which mostly dodges the question of whether to initialize variables ahead of use.

Sounds good to me. I'll look at moving some of those declarations to the point of first use and will make sure to avoid anything past C99/gnu.

To make your program portable, you should check for both codes and treat them the same.

Perfect, thanks for digging up concrete guidance. Portability was my concern too, I knew the values were the same on my machine but it seemed like a risky assumption to make in a broader sense.