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

Add parentheses to fix an unsafe macro. #486

Open
wants to merge 1 commit into
base: libpng16
Choose a base branch
from

Conversation

irwir
Copy link

@irwir irwir commented Jul 7, 2023

Static analysis flags PNG_FORMAT_NUMBER macro as unsafe.
Parentheses ensure correct evaluation in case buffer argument was a complex expression.

@jbowler
Copy link
Contributor

jbowler commented Nov 1, 2023

I wrote it that way for good reasons. The only argument permitted is an lvalue array name. There is some chance that if the macro is abused by a future maintainer it will produce a compile error.

@jbowler
Copy link
Contributor

jbowler commented Nov 12, 2023

Like I said. To put in another way don't fix things that work.

@irwir
Copy link
Author

irwir commented Nov 13, 2023

jbowler, thanks for your remarks.
The macro is unsafe. Normally these warnings should be silenced.

Expecting a compilation error in case of misuse is a poor protection.
For example, a pointer instead of an array name compiles quietly and might even work in some cases.

@jbowler
Copy link
Contributor

jbowler commented Nov 13, 2023

Silencing a warning by changing the semantics of the code is not correct. The macro requires an argument of type (char{]); find me a place where the macro is used with an argument which is not only of that type but also not a local C array of char. Your change is not an appropriate way to silence the warning issued by your compiler. I suggest you take a look at the ways of silencing warnings. libpng already has an extensive list for GCC, but it requires continuous maintenance; the maintainers build libpng with -Wall -Wextra -Werror and then with those specific warnings disabled.

You change is not safe; it might not change the result with current code but that all uses a simple variable name. It would change the semantics of an editing or understanding error.

Now it's not perfect, so if you can provide an analysis which suggests the likely editing errors (i.e. typographical errors made by a maintainer of the code) can be detected using a different approach then that's more interesting.

But the bottom line is still this: a compiler warning is NOT a bug. Compilers, particular GCC generate warnings with every new dawn. They go away by sunset.

@ctruta

Making code changes to established working and substantially unmaintained code just because of a new compiler warning is likely to lead to new bugs not fix old ones. Sure, if someone can show that there really is a bug, perhaps as a result of an analysis caused by a compiler warning, that's useful. But that isn't what is happening here or in several other bug reports that should simply be closed.

libpng 1.6 is substantially working and so far no one has found a significant code bug for a while. Sure lots have been reported and some have been fixed, particularly from fuzzers, but people are resorting to coming up with compiler warnings or convoluted cases involving test programs which are not part of a binary release. These are not bugs.

libpng is stable. Don't destabilize it! Any new code other than hardware support, which is isolated from the main code, should go in libpng1.8; whoever develops that branch is free to mess with things like this, or, for that anything.

@irwir
Copy link
Author

irwir commented Nov 14, 2023

Silencing a warning by changing the semantics of the code is not correct.

@jbowler, please care to explain how semantics was easily changed with an additional pair of parentheses.

@irwir
Copy link
Author

irwir commented Nov 23, 2023

@jbowler
The first two words in the description of this PR are Static analisys, and the title has the word fix. Once again: fix, not silence.
This kind of warning cannot be silenced with compilation options - simply because it was a different tool.
All this was misread, and the concept of unsafe macro misinterpreted.
Please learn the basics, and lower the level of arrogance.

@irwir
Copy link
Author

irwir commented Dec 7, 2023

By the way, this tiny issue was promised to be fixed in the #216 - closed only partially completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants