5 ms·

The author's article about comparison functions linked in the blog post is worth the read Regarding the OpenSSL example presented here, wouldn't any decent IDE

by enlyth 5y ago

The author's article about comparison functions linked in the blog post is worth the read Regarding the OpenSSL example presented here, wouldn't any decent IDE catch an unused parameter in a function? Why is a separate static analyzer necessary for this

wwn_se 5y ago
I think the point is if you integrate the analyzer in the workflow its hard to ignore. The ide warnings are easy to miss sometimes.
- tialaramex 5y ago
  The Right Thing™ for a project that actually has multiple contributors is that the CI pipeline rejects code that based on the compiler diagnostics is either definitely wrong or, if it was correct, should be annotated in a way the compiler can understand and stop emitting the diagnostic. It doesn't matter whether one or another contributor has this switched off or ignored it. That is, committing this code should have had the same impact as if it was missing the semi-colon, the library doesn't build, tree is on fire, fix before you do new work. Of course if your code is in sufficiently bad state, you might find it's frustrating to have say 500 bugs to solve before you can get the CI pipeline to output artefacts. I think that means you didn't have good software and you must fix those bugs first, but if you're convinced the software is good it can be tempting to instead disable the diagnostics telling you otherwise and press on. You have 178 seconds to live: https://www.youtube.com/watch?v=b7t4IR-3mSo https://www.youtube.com/watch?v=b7t4IR-3mSo
jraph 5y ago
> Why is a separate static analyzer necessary for this Because this is a nail this co-founder of PVS-Studio saw while holding the hammer he wants to sell. By the way, it seems to me this error does not have much to do with comparison functions, it is a mistake that can be made in many kinds of functions. Unused parameters are warned against in C (hence all the UNUSED macro hacks) and no static analyzers are going to save you if you don't address compiler warnings, they are just going to add more warnings to that pile of warnings you already have (or maybe don't get because the right warnings are not enabled). (I'm sure their blog is full of cases a static analyzer would handle which compilers won't warn you for though) Still surprising to see such an error in OpenSSL and like others here I would be interested to know why it was not caught.
- choeger 5y ago
  I think comparison functions are a prime "use case" for this kind of error because it will pass the type check easily and comparison is often somewhat symmetric, so you will do a lot of copy and paste. Only unit or property tests could catch this (and a compiler warning, of course). The example here is of course extreme, but I bet you will find something like this quite often, especially in OO code like Java.
- NohatCoder 5y ago
  Last time OpenSSL was in the news for a bug that should generate a warning I took a cursory look at some of the code, it was stuffed with stuff that would generate similar warnings. So at that time I concluded that going through all the warnings would be a pretty big job, and that is likely why nobody did so. On the more speculative front, it might be that some maintainer is against taking whitespace patches, that would probably make anyone who tries give up pretty quickly. I'm a bit skeptical of the claim "The code quality is excellent", if it was this would be a lone warning emitted by the compiler, and surely it would have been fixed then.
  - vlovich123 5y ago
    “The code quality is excellent” is either the author hedging pissing people off who are fans of OpenSSL or their analyzer doesn’t spot a lot of issues and thus they conclude it’s excellent for those reasons.
    - adrian_b 5y ago
      Actually the author said that the “The code quality is excellent” specifically about the new version 3.0 in contrast with the older OpenSSL versions, where he could find much more problems. I have not looked yet at the new version, which has a very large number of changes, but it is quite possible that the author is right and that the rewritten code is very good.
umanwizard 5y ago
A lot of people don’t use IDEs to write C. That said, I believe the two mainstream C compilers (GCC and Clang) can both be configured to emit a warning here.
PVS-Studio 5y ago
If a diagnostic issues warnings to unused arguments - it's a weak diagnostic. PVS-Studio acts differently. Take a look at my clarifying comment below or above (not sure where it would be when you read it).

by enlyth 5y ago

wwn_se 5y ago

I think the point is if you integrate the analyzer in the workflow its hard to ignore. The ide warnings are easy to miss sometimes.

tialaramex 5y ago
The Right Thing™ for a project that actually has multiple contributors is that the CI pipeline rejects code that based on the compiler diagnostics is either definitely wrong or, if it was correct, should be annotated in a way the compiler can understand and stop emitting the diagnostic. It doesn't matter whether one or another contributor has this switched off or ignored it. That is, committing this code should have had the same impact as if it was missing the semi-colon, the library doesn't build, tree is on fire, fix before you do new work. Of course if your code is in sufficiently bad state, you might find it's frustrating to have say 500 bugs to solve before you can get the CI pipeline to output artefacts. I think that means you didn't have good software and you must fix those bugs first, but if you're convinced the software is good it can be tempting to instead disable the diagnostics telling you otherwise and press on. You have 178 seconds to live: https://www.youtube.com/watch?v=b7t4IR-3mSo https://www.youtube.com/watch?v=b7t4IR-3mSo

jraph 5y ago

> Why is a separate static analyzer necessary for this Because this is a nail this co-founder of PVS-Studio saw while holding the hammer he wants to sell. By the way, it seems to me this error does not have much to do with comparison functions, it is a mistake that can be made in many kinds of functions. Unused parameters are warned against in C (hence all the UNUSED macro hacks) and no static analyzers are going to save you if you don't address compiler warnings, they are just going to add more warnings to that pile of warnings you already have (or maybe don't get because the right warnings are not enabled). (I'm sure their blog is full of cases a static analyzer would handle which compilers won't warn you for though) Still surprising to see such an error in OpenSSL and like others here I would be interested to know why it was not caught.

choeger 5y ago
I think comparison functions are a prime "use case" for this kind of error because it will pass the type check easily and comparison is often somewhat symmetric, so you will do a lot of copy and paste. Only unit or property tests could catch this (and a compiler warning, of course). The example here is of course extreme, but I bet you will find something like this quite often, especially in OO code like Java.
NohatCoder 5y ago
Last time OpenSSL was in the news for a bug that should generate a warning I took a cursory look at some of the code, it was stuffed with stuff that would generate similar warnings. So at that time I concluded that going through all the warnings would be a pretty big job, and that is likely why nobody did so. On the more speculative front, it might be that some maintainer is against taking whitespace patches, that would probably make anyone who tries give up pretty quickly. I'm a bit skeptical of the claim "The code quality is excellent", if it was this would be a lone warning emitted by the compiler, and surely it would have been fixed then.
- vlovich123 5y ago
  “The code quality is excellent” is either the author hedging pissing people off who are fans of OpenSSL or their analyzer doesn’t spot a lot of issues and thus they conclude it’s excellent for those reasons.
  - adrian_b 5y ago
    Actually the author said that the “The code quality is excellent” specifically about the new version 3.0 in contrast with the older OpenSSL versions, where he could find much more problems. I have not looked yet at the new version, which has a very large number of changes, but it is quite possible that the author is right and that the rewritten code is very good.

umanwizard 5y ago

A lot of people don’t use IDEs to write C. That said, I believe the two mainstream C compilers (GCC and Clang) can both be configured to emit a warning here.

PVS-Studio 5y ago

If a diagnostic issues warnings to unused arguments - it's a weak diagnostic. PVS-Studio acts differently. Take a look at my clarifying comment below or above (not sure where it would be when you read it).