Improved handling of function parameter validation #13975
Conversation
|
Okay, thanks for the feedback everyone! I think this is a much improved approach. @icculus, @sezero, @madebr, @smcv, @AntTheAlchemist, @mechakotik, @bjorn |
slouken
force-pushed
the
param-checks2
branch
2 times, most recently
from
3898212 to
fab019c
Compare
|
I like the fact that this change reverts the behavior to mostly match SDL2 in terms of parameter checking, by making the slow object validation an opt-in. It's also nice that it's still possible to compile the checks out entirely. I agree with mechakotik's comment that a runtime NULL-check bypass doesn't add any value (in terms of performance, though it might have been useful for debugging purposes). It seems to me that in case of |
That's a good point, I'll go ahead and do that, thanks! |
slouken
force-pushed
the
param-checks2
branch
2 times, most recently
from
5f78ed2 to
ae65c10
Compare
|
@bjorn, that made the diff much more readable and less intrusive, thanks! |
|
I think it's a good idea to explicitly inline the "fast checks" part of SDL_INLINE bool SDL_ObjectValid(void *object, SDL_ObjectType type)
{
if (!object) {
return false;
}
if (!SDL_object_validation) {
return true;
}
return CheckObjectInHashTable(object, type);
}Also, why would we spend resources maintaining |
slouken
force-pushed
the
param-checks2
branch
from
ae65c10 to
8173b61
Compare
Good idea, I'll go ahead and do that.
Because it allows object leak tracking in the default case when full object validation isn't enabled.
This allows an application to set it at startup after other code (possibly in a third party library) has been run and potentially triggered this initialization. |
slouken
force-pushed
the
param-checks2
branch
2 times, most recently
from
3b139ee to
b8500a8
Compare
|
Okay, I've warmed up on this in the latest revisions. It's pretty good looking now! |
slouken
force-pushed
the
param-checks2
branch
2 times, most recently
from
33ecadd to
bbb841b
Compare
|
@madebr, do you see any reason why the Emscripten build would abort in the video_raiseWindow() test? |
slouken
force-pushed
the
param-checks2
branch
from
bbb841b to
c4d77ae
Compare
I cannot reproduce locally. Can you rebase your branch on top of current master, and add |
slouken
force-pushed
the
param-checks2
branch
from
c4d77ae to
676b365
Compare
Done! |
|
I can reproduce the error (occasionally) using chromium. Other times, the test exits fine with error code 0, but this error gets printed after exit: Reproducer steps (you need to rebase one more time on top of current main because of a
|
slouken
force-pushed
the
param-checks2
branch
from
676b365 to
f98c509
Compare
|
Okay, I've rebased and rebuilt. Does the error happen if you run |
No, sadly it does not. I can reproduce errors locally by adding this to code to the SDL_{malloc,calloc,free}` #ifdef __EMSCRIPTEN__
EM_ASM({
checkStackCookie();
});
#endifAnyhow, I think the error is in SDL and triggered by Reproducer: |
slouken
force-pushed
the
param-checks2
branch
from
f98c509 to
4ed44fa
Compare
Ah, thank you for the stack trace, that was very helpful. Also, I see now that we were relying on the object validity check to prevent a crash here before. I wonder how many other people are going to run into similar problems? |
slouken
force-pushed
the
param-checks2
branch
from
4ed44fa to
0856a2c
Compare
SDL supports the following use cases:
* Normal operation with fast parameter checks (default):
SDL_SetHint(SDL_HINT_INVALID_PARAM_CHECKS, "1");
* Object parameters are checked for use-after-free issues:
SDL_SetHint(SDL_HINT_INVALID_PARAM_CHECKS, "2");
* Enable full validation, plus assert on invalid parameters:
#define SDL_ASSERT_INVALID_PARAMS
* Disable all parameter validation:
#define SDL_DISABLE_INVALID_PARAMS
slouken
force-pushed
the
param-checks2
branch
from
0856a2c to
ee72442
Compare
testsprite also has an obvious bug. So there will be others. |
🙈 I found one in SDL_mixer.c: Shall I create a new issue for this over at SDL_mixer? |
|
Another one, when plugging in a gamepad. To be fair, I don't think there are many of these. This is a good thing, right? It's exposing potential internal SDL bugs? We can stamp them out as we find them. |
|
On Android: |
By default there are still fast null checks, which would likely prevent all the above issues, right? I don't have time to look into each of them, but it seems like they are being found in optimized builds with checks entirely disabled? I do wonder a little whether the option to remove these checks entirely was a good idea. Now all code will need to be adjusted to handle the case where these checks have been compiled out, which may in many cases cause needless double-checking of the parameters in a default build (checking the parameter both outside and inside the function). It may be worth it for optimized builds, but has any difference in performance actually been demonstrated? (as opposed to disabling just |
Yep. Well, we might as well fix these. Can you provide a PR for this? |
I just pushed it to git, since it was a trivial fix. |
Special thanks to @mechakotik for their investigation and first pass of this PR.
SDL supports the following use cases:
SDL_SetHint(SDL_HINT_INVALID_PARAM_CHECKS, "1");SDL_SetHint(SDL_HINT_INVALID_PARAM_CHECKS, "2");#define SDL_ASSERT_INVALID_PARAMS#define SDL_DISABLE_INVALID_PARAMSCloses #13213
Closes #13943