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

[RPMLint] Program returns random data in a function (RenderTarget.cpp) #935

Closed
susnux opened this Issue Aug 1, 2015 · 20 comments

Comments

Projects
None yet
5 participants
@susnux
Contributor

susnux commented Aug 1, 2015

I am trying to build SFML 2.3.1 for openSUSE, but build failed with RPMLint report:

I: Program returns random data in a function
E: sfml2 no-return-in-nonvoid-function SFML-2.3.1/src/SFML/Graphics/RenderTarget.cpp:56, 67

Because the functions:
sf::Uint32 factorToGlConstant(sf::BlendMode::Factor blendFactor)
and
sf::Uint32 equationToGlConstant(sf::BlendMode::Equation blendEquation)
do not have a default return.

I use a simple patch, if you want to, you can use it.
Direct link to patch: https://build.opensuse.org/source/games/sfml2/fix-upstream-no-return-in-nonvoid-function.patch?rev=24d1fc187f50849c3826c094f028057b

Edit: Updated link to patch.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 3, 2015

Member

Good catch. A pull request would help us to integrate this easier, though. ;)

Member

TankOs commented Aug 3, 2015

Good catch. A pull request would help us to integrate this easier, though. ;)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 3, 2015

Member

For the future, please refer to the contribution guidelines before creating issues.


I don't get why C++ compilers and static analyzers are always stupid when it comes to switch statements. In Java, for example, they're much more useful. We obviously handle every valid case, there's definitely no need to emit an error for correct C++ code.

I'm totally aware that you can force an integer into an enum that is not listed as an enumerator, but that's absolutely not a problem of the code that handles the switch statement. I don't think we should add silly code to catch undefined behavior -- or if we do, at least in the form of a precondition/assert.

Because one big problem that default introduces is that it catches all the cases even if they aren't handled. That is, if the enum grows in the future, the few smart static analyzers/compilers can tell you that you forgot to adapt the switch statement accordingly. However, when there's a default label, they think you intended to handle the missing cases, and you get runtime errors instead.

Member

Bromeon commented Aug 3, 2015

For the future, please refer to the contribution guidelines before creating issues.


I don't get why C++ compilers and static analyzers are always stupid when it comes to switch statements. In Java, for example, they're much more useful. We obviously handle every valid case, there's definitely no need to emit an error for correct C++ code.

I'm totally aware that you can force an integer into an enum that is not listed as an enumerator, but that's absolutely not a problem of the code that handles the switch statement. I don't think we should add silly code to catch undefined behavior -- or if we do, at least in the form of a precondition/assert.

Because one big problem that default introduces is that it catches all the cases even if they aren't handled. That is, if the enum grows in the future, the few smart static analyzers/compilers can tell you that you forgot to adapt the switch statement accordingly. However, when there's a default label, they think you intended to handle the missing cases, and you get runtime errors instead.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 3, 2015

Member

The error is valid in my opinion and the error is not directly about the switch statement.

The issue is that not all possible code paths will end up returning a value for the function. Sure, we don't call the function with a different value than the ones in the enum, yet an enum is basically just an integer and it's legal C++ code to cast and pass in any integer.

The problem right now is that, if someone would ever pass in an integer that is not in the enum, the function will probably silently fail and return random data. Sure you can argue that the user asked for UB when passing in a wrong value, but failing silently is in my opinion bad.

On IRC it was suggested to put an assert after the switch statement. And one could still add a return, even though it then would never be reached.

Member

eXpl0it3r commented Aug 3, 2015

The error is valid in my opinion and the error is not directly about the switch statement.

The issue is that not all possible code paths will end up returning a value for the function. Sure, we don't call the function with a different value than the ones in the enum, yet an enum is basically just an integer and it's legal C++ code to cast and pass in any integer.

The problem right now is that, if someone would ever pass in an integer that is not in the enum, the function will probably silently fail and return random data. Sure you can argue that the user asked for UB when passing in a wrong value, but failing silently is in my opinion bad.

On IRC it was suggested to put an assert after the switch statement. And one could still add a return, even though it then would never be reached.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 3, 2015

Member

"Better safe than sorry" is everything I want to say. :)

Member

TankOs commented Aug 3, 2015

"Better safe than sorry" is everything I want to say. :)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 3, 2015

Member

an enum is basically just an integer and it's legal C++ code to cast and pass in any integer.

No -- it's unspecified behavior, meaning you can't portably rely on a behavior. And you can especially not pass in any integer, the range of valid ones is clearly defined by the standard. This apart from the fact that it makes no sense in most cases, as the whole point of using enum is to define a finite, bounded set of states.

The problem right now is that, if someone would ever pass in an integer that is not in the enum, the function will probably silently fail and return random data. Sure you can argue that the user asked for UB when passing in a wrong value, but failing silently is in my opinion bad.

Yes, it's bad, but as I mentioned, it's not the responsibility of this function to handle user errors. We don't do this in all the other functions that work with enums, either. And if the compilers and static analyzers didn't complain here, nobody would even remotely consider this an issue 😉

Anyway, there's probably no way around this, we have to comply with the tools. Because it's not the first time I have been annoyed by this circumstance, I even have some code that deals with it, I can write a PR if you like 😀

Member

Bromeon commented Aug 3, 2015

an enum is basically just an integer and it's legal C++ code to cast and pass in any integer.

No -- it's unspecified behavior, meaning you can't portably rely on a behavior. And you can especially not pass in any integer, the range of valid ones is clearly defined by the standard. This apart from the fact that it makes no sense in most cases, as the whole point of using enum is to define a finite, bounded set of states.

The problem right now is that, if someone would ever pass in an integer that is not in the enum, the function will probably silently fail and return random data. Sure you can argue that the user asked for UB when passing in a wrong value, but failing silently is in my opinion bad.

Yes, it's bad, but as I mentioned, it's not the responsibility of this function to handle user errors. We don't do this in all the other functions that work with enums, either. And if the compilers and static analyzers didn't complain here, nobody would even remotely consider this an issue 😉

Anyway, there's probably no way around this, we have to comply with the tools. Because it's not the first time I have been annoyed by this circumstance, I even have some code that deals with it, I can write a PR if you like 😀

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 3, 2015

Member

Wouldn't call it a problem to create dummy return values for code analyzers (probably with an added assert).

Actually MSVC only detects two such issues in SFML, both with the ...ToGlConstant() helpers. There are a few more warnings (deprecated or unsave calls) though.

Member

MarioLiebisch commented Aug 3, 2015

Wouldn't call it a problem to create dummy return values for code analyzers (probably with an added assert).

Actually MSVC only detects two such issues in SFML, both with the ...ToGlConstant() helpers. There are a few more warnings (deprecated or unsave calls) though.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 4, 2015

Member

IMHO it will still make the code safer. It doesn't cost us anything but will especially make refactorings easier, IF that happens.

Member

TankOs commented Aug 4, 2015

IMHO it will still make the code safer. It doesn't cost us anything but will especially make refactorings easier, IF that happens.

@susnux

This comment has been minimized.

Show comment
Hide comment
@susnux

susnux Aug 4, 2015

Contributor

My personal opinion is: In a library I would ever add a default, in a normal program it is not a problem (you can be safe, if you not use a cast). But in a library you do not know how stupid your user is ;-)
And it adds more security, without much more work.
But that is only my opinion and I am not a SFML-developer.

PS: I think(!) even with a default you can check if you missed an enum case in switch with this gcc flag:
-Wswitch-enum

Contributor

susnux commented Aug 4, 2015

My personal opinion is: In a library I would ever add a default, in a normal program it is not a problem (you can be safe, if you not use a cast). But in a library you do not know how stupid your user is ;-)
And it adds more security, without much more work.
But that is only my opinion and I am not a SFML-developer.

PS: I think(!) even with a default you can check if you missed an enum case in switch with this gcc flag:
-Wswitch-enum

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 5, 2015

Member

@susnux This patch should be alright. Can you be bothered with creating a PR, so that we can apply it easily together with your author info?

Member

TankOs commented Aug 5, 2015

@susnux This patch should be alright. Can you be bothered with creating a PR, so that we can apply it easily together with your author info?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 5, 2015

Member

[MarioLiebisch] Wouldn't call it a problem to create dummy return values for code analyzers (probably with an added assert).
[TankOs] It doesn't cost us anything

Yes, it does:

[Bromeon] Because one big problem that default introduces is that it catches all the cases even if they aren't handled. That is, if the enum grows in the future, the few smart static analyzers/compilers can tell you that you forgot to adapt the switch statement accordingly. However, when there's a default label, they think you intended to handle the missing cases, and you get runtime errors instead.

We take this disadvantage only to satisfy a badly-written tool, and we write redundant code to "fix" something that has been 100% correct, always -- at the cost of potentially introducing bugs in the future.


@susnux This patch should be alright. Can you be bothered with creating a PR, so that we can apply it easily together with your author info?

The patch is not good, it still ignores the error. sf::err() is only diagnostic output, the program continues to run even in Debug mode.

As mentioned above:

Because it's not the first time I have been annoyed by this circumstance, I even have some code that deals with it, I can write a PR if you like 😀

But I want to be sure that you're fully aware of the disadvantages, which do exist, as outlined above. Hiding bugs in future enum extensions is not something we should carelessly do, and it's not only about this switch/case specifically.

Some further opinions would be nice, too.

Member

Bromeon commented Aug 5, 2015

[MarioLiebisch] Wouldn't call it a problem to create dummy return values for code analyzers (probably with an added assert).
[TankOs] It doesn't cost us anything

Yes, it does:

[Bromeon] Because one big problem that default introduces is that it catches all the cases even if they aren't handled. That is, if the enum grows in the future, the few smart static analyzers/compilers can tell you that you forgot to adapt the switch statement accordingly. However, when there's a default label, they think you intended to handle the missing cases, and you get runtime errors instead.

We take this disadvantage only to satisfy a badly-written tool, and we write redundant code to "fix" something that has been 100% correct, always -- at the cost of potentially introducing bugs in the future.


@susnux This patch should be alright. Can you be bothered with creating a PR, so that we can apply it easily together with your author info?

The patch is not good, it still ignores the error. sf::err() is only diagnostic output, the program continues to run even in Debug mode.

As mentioned above:

Because it's not the first time I have been annoyed by this circumstance, I even have some code that deals with it, I can write a PR if you like 😀

But I want to be sure that you're fully aware of the disadvantages, which do exist, as outlined above. Hiding bugs in future enum extensions is not something we should carelessly do, and it's not only about this switch/case specifically.

Some further opinions would be nice, too.

@susnux

This comment has been minimized.

Show comment
Hide comment
@susnux

susnux Aug 5, 2015

Contributor

@Bromeon No, it costs you nothing. I tried it myself and even if you have a "default", a simple gcc flag ("-Wswitch-enum") warns about missing case for enum.
See yourself (warning output line 54++):
http://pastie.org/10332164

The patch is not good, it still ignores the error. sf::err() is only diagnostic output, the program continues to run even in Debug mode.

Hm, that's a matter of opinion. In my opinion the user (better the developer) should be informed about the error, but for the user the program using the library should not crash.
But that is only my opinion.

Contributor

susnux commented Aug 5, 2015

@Bromeon No, it costs you nothing. I tried it myself and even if you have a "default", a simple gcc flag ("-Wswitch-enum") warns about missing case for enum.
See yourself (warning output line 54++):
http://pastie.org/10332164

The patch is not good, it still ignores the error. sf::err() is only diagnostic output, the program continues to run even in Debug mode.

Hm, that's a matter of opinion. In my opinion the user (better the developer) should be informed about the error, but for the user the program using the library should not crash.
But that is only my opinion.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 6, 2015

Member

@Bromeon
I wonder why you always bring default: to the table. The patch is not adding default: to the switch, it's just adding a return after the switch, so that all possible code paths have a return statement (yes, I know, the path is not possible because all cases are handled, but lint tools often do not go that far, and I still think that it even increases readability).

That means you'll still be warned if there's a new case value that's not handled in the switch.

Member

TankOs commented Aug 6, 2015

@Bromeon
I wonder why you always bring default: to the table. The patch is not adding default: to the switch, it's just adding a return after the switch, so that all possible code paths have a return statement (yes, I know, the path is not possible because all cases are handled, but lint tools often do not go that far, and I still think that it even increases readability).

That means you'll still be warned if there's a new case value that's not handled in the switch.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 6, 2015

Member

Regarding the error message: Right now, we are doing nothing, and you said yourself that the path will never be reached. This is a safe guard, that will at least print something out if the rare case happens that the switch did not handle a case (aka an invalid value was passed to the function).

I'm fine with adding an assert on top.

Member

TankOs commented Aug 6, 2015

Regarding the error message: Right now, we are doing nothing, and you said yourself that the path will never be reached. This is a safe guard, that will at least print something out if the rare case happens that the switch did not handle a case (aka an invalid value was passed to the function).

I'm fine with adding an assert on top.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 6, 2015

Member

The patch is not adding default: to the switch, it's just adding a return after the switch

That's true, sorry, I was still thinking in default terms. In languages like Java where you get heavy IDE support, I've seen it quite a lot that people rely on these warnings and simply forget to adapt switch/case statements when a default "eats" everything.

I tried it myself and even if you have a "default", a simple gcc flag ("-Wswitch-enum") warns about missing case for enum.

But that's also broken; in fact, the whole analysis around switch/case is awful in C++. There's no point in giving a warning when multiple cases are put together. Remember that the intention of default: is explicitly "all the other cases", not "all the invalid cases that don't appear in correct C++".

In my opinion the user (better the developer) should be informed about the error, but for the user the program using the library should not crash.

Informing the developer would be achieved with simply adding assert(false) -- it doesn't do anything for the user, but it helps the developer find the problem immediately. Ignoring or "fixing" (i.e. hiding) logic errors by returning arbitrary values is really not ideal.


Don't forget that this whole topic is something we recently discussed, and the conclusion was to leave things as-is for the moment, but to agree on an library-wide approach regarding assertions and checks in the future, instead of handling every request on its own. See this post in particular.

Member

Bromeon commented Aug 6, 2015

The patch is not adding default: to the switch, it's just adding a return after the switch

That's true, sorry, I was still thinking in default terms. In languages like Java where you get heavy IDE support, I've seen it quite a lot that people rely on these warnings and simply forget to adapt switch/case statements when a default "eats" everything.

I tried it myself and even if you have a "default", a simple gcc flag ("-Wswitch-enum") warns about missing case for enum.

But that's also broken; in fact, the whole analysis around switch/case is awful in C++. There's no point in giving a warning when multiple cases are put together. Remember that the intention of default: is explicitly "all the other cases", not "all the invalid cases that don't appear in correct C++".

In my opinion the user (better the developer) should be informed about the error, but for the user the program using the library should not crash.

Informing the developer would be achieved with simply adding assert(false) -- it doesn't do anything for the user, but it helps the developer find the problem immediately. Ignoring or "fixing" (i.e. hiding) logic errors by returning arbitrary values is really not ideal.


Don't forget that this whole topic is something we recently discussed, and the conclusion was to leave things as-is for the moment, but to agree on an library-wide approach regarding assertions and checks in the future, instead of handling every request on its own. See this post in particular.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 6, 2015

Member

So do we close this or keep it open or close it and create an issue on "refactoring" assert usage?

Member

eXpl0it3r commented Aug 6, 2015

So do we close this or keep it open or close it and create an issue on "refactoring" assert usage?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 6, 2015

Member

on an library-wide approach regarding assertions and checks in the future, instead of handling every request on its own

That's correct, and mainly the reason why I did not come up with "Please add an assert on top". What do you guys think of just keeping this simple, apply the patch + an assert on top? It makes sense now, and it'll probably make sense when we apply a library-wide strategy for error reporting.

Member

TankOs commented Aug 6, 2015

on an library-wide approach regarding assertions and checks in the future, instead of handling every request on its own

That's correct, and mainly the reason why I did not come up with "Please add an assert on top". What do you guys think of just keeping this simple, apply the patch + an assert on top? It makes sense now, and it'll probably make sense when we apply a library-wide strategy for error reporting.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 6, 2015

Member

What do you guys think of just keeping this simple, apply the patch + an assert on top? It makes sense now, and it'll probably make sense when we apply a library-wide strategy for error reporting.

Okay 😀
"Let's not open an office here" 😛

Member

Bromeon commented Aug 6, 2015

What do you guys think of just keeping this simple, apply the patch + an assert on top? It makes sense now, and it'll probably make sense when we apply a library-wide strategy for error reporting.

Okay 😀
"Let's not open an office here" 😛

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 7, 2015

Member

:D

Member

TankOs commented Aug 7, 2015

:D

@susnux

This comment has been minimized.

Show comment
Hide comment
@susnux

susnux Aug 13, 2015

Contributor

I have created a pull request to close this issue, I applied the patch and added an assert.
I think this was the common consent.

Contributor

susnux commented Aug 13, 2015

I have created a pull request to close this issue, I applied the patch and added an assert.
I think this was the common consent.

@TankOs TankOs added s:accepted and removed s:undecided labels Aug 14, 2015

@TankOs TankOs added this to the 2.3.2 milestone Aug 14, 2015

susnux added a commit to susnux/SFML that referenced this issue Aug 17, 2015

eXpl0it3r added a commit that referenced this issue Aug 24, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 24, 2015

Member

Fixed in 0f1dc5a on branch 2.3.x

Member

eXpl0it3r commented Aug 24, 2015

Fixed in 0f1dc5a on branch 2.3.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment