void RM_FreeString(RedisModuleCtx *ctx, RedisModuleString *str) {
decrRefCount(str);
if (ctx != NULL) autoMemoryFreed(ctx,REDISMODULE_AM_STRING,str);
}
As RM_FreeString() does not expect NULL pointer, modules must be sure they are not passing NULL pointer in. As far as I can see, it causes lots of NULL checks inside modules. We can just add a null check there if (str == NULL) return.
Also, it'd be inline with free()/RM_Free() as these functions accept NULL pointers. (Maybe we should consider this for other free family functions, RM_FreeCallReply() etc.)
Comment From: madolson
I am always a fan of free functions not doing null checks.
Comment From: oranagra
i agree (didn't notice that till now). note that RM_Free and RM_Realloc do support NULL pointers.
Comment From: oranagra
the only thing that worries me is that older redis versions won't have that support. so someone writing a module and testing it on the latest, will miss this and the module would crash on old version (or worse, have rare crashes in some edge cases).
Comment From: tezc
Yes, it will require proper version check. Also, if a module wants to support older Redis versions, it won't be able to use this. So, this is for modules that target newer Redis versions.
Comment From: oranagra
What worries me is modules that only test with the latest, and won't notice the missing check required for older versions. @madolson @yossigo WDYT?
Comment From: yossigo
@oranagra I agree, this is already the case with flags and API functions but this one is more easy to miss.
The goal of this change is to minimize boilerplate, but that can only be done if we're willing to break backward compatibility. Maybe we need a more standard and formalized way for modules to handle version dependency checks.
Comment From: zuiderkwast
What worries me is modules that only test with the latest, and won't notice the missing check required for older versions.
What if we add a note in the docs of RedisModule_FreeString that null pointers are accepted in Redis 7.1 but older Redis will crash... Is that enough?
Even if you test the module, it can still happen that you didn't test a corner case where null is passed to the function. This will cause a crash in old redis, but it doesn't harm fixing it in new redis imho.
Comment From: oranagra
maybe it'll help.. but in many cases i'm guessing it won't.. someone will assume it works, or simply not notice he has a rare flow that hits this, on the latest, where all the tests are made it'll work, and on older versions it won't but no one will notice.
Comment From: slavak
It's a bit ugly, but we could add the NULL-check as a bit of macro-magic wrapping RedisModule_FreeString in the header file. That way modules are guaranteed the simplified API at compile time, and don't depend on the version of Redis they get loaded into.
Comment From: oranagra
that's indeed ugly, but considering that redismodule.h is probably always embedded in the module's repo, i suppose it's a viable solution with no side effects.
Comment From: tezc
In addition to macro check, we can still add null check into the function itself. Even though we'll never hit that code, one can read that function and realize null values are accepted without checking the header. I guess it'll be less confusing this way.
Overall, I'm not sure if it is worth this hack :)
Comment From: slavak
@tezc I think adding it to the function body would resurface the concern Oran brought up originally. Someone who hasn't updated their bundled header may test with the latest Redis and see no issues, but have a lurking bug when loading into an older Redis version.
I'll leave the decision of whether the ugliness is worth it up to you guys. :)
Comment From: tezc
I always take a look at function implementation before using it. It'd take some time for me to figure out there is a hidden null check in the header. I can see myself spending hours trying to debug it :) So, although macro check in the header file will solve the problem, I think it'll hurt readability.
Comment From: oranagra
we can solve that with a bold comment in the code.
Comment From: tezc
Yeah, maybe. I don't have a strong opinion anyway. If you guys think this is good enough, let's do it.
Comment From: slavak
I was thinking we could combine the two approaches: 1. A macro in the header file to properly handle NULL arguments. 2. A branch in the actual code that would accept NULL arguments but print a visible warning to the screen. Calling the function with a NULL argument would be safe, but someone only testing a module with the newest Redis should still be aware they'd have a problem with older versions.
(1) would make for a nicer API for anyone writing C/C++, while (2) would at least put us on a path to fully switching to the nicer behaviour.
Comment From: slavak
I opened a simple PR for this change.
In the end I opted to forego the macro magic, since it's not very clear, and would not benefit people working in languages other than C/C++. I added an info-level message to alert the user when RM_FreeString is called with a NULL pointer. Hopefully it's not too intrusive but good enough to alert developers that only test on newest Redis that they might have an issue.
In practice this probably means most module developers won't choose to rely on this functionality even if they only plan to support the latest version of Redis, as spewing warnings to the log isn't the best. But hopefully it at least creates some kind of path towards properly integrating this functionality: When the versions that crash on this are old enough we could get rid of the message without breaking modules entirely.
(I added a test case to cover this out of an abundance of caution. Not sure if it's really worthwhile having it there.)
Comment From: oranagra
i'm still not happy about the compromises here. arguably, dropping the preprocessor idea brings us back to a case that a module that's using a new version will get away from passing NULL, and then crash on an older version.
the idea of using proprocessor was that we shift this check to the module "code base", so it'll remain constant no matter which redis version is used to load it.
maybe the right plan to overcome this is to use a macro for 8 years, and then add it to redis itself? or just leave it as is.. some things you cannot fix..
Comment From: tezc
Personal opinion, either we should just add a null check to the source code or we shouldn't fix this issue. It is just a teeny-tiny feature, not critical at all but its solution requires complexity / trick in the code. (even if it is just a one-line macro check). I don't think it is worth it. For a critical issue, macro check might be a good alternative but for this one, I'm not sure.
Comment From: slavak
I tend to gravitate towards @tezc's position over time. Macro magic feels a bit over-complicated for such a small quality-of-life feature, particularly when it still wouldn't protect many users. (e.g.: Rust modules.)
I'm assuming any module developer would at least need to glance at the redis-server output while testing, and a screenful of logs is pretty hard to miss- so this felt like a reasonable compromise. Are you worried this isn't the case?
Comment From: oranagra
yes, i'm worried they'll miss the logs, or maybe they won't even reach the rare code path in which some NULL pointer isn't checked. and then on this rare case, a user running the latest version will get a flooded log file (which we can indeed throttle), but what's worse is that i'll get crashes on an old server. i.e. without this fix, the crashes will also be visible on an new server, even if there was just one occurrence (which could easily be missed in the logs).
i think we have no choice but accept this flaw and leave it as it is.
Comment From: slavak
Maybe we need a general API that allows modules to declare a minimum supported Redis version? In this case we could enable the new behaviour only for modules that don't need to work on older versions, but I imagine there would be more interesting use cases as well.
Comment From: oranagra
so far it was the other way around. modules would detect which version they're running in, or what's supported, and made adjustments in their code accordingly. or just abort loading if the redis version is unsuitable. it feels a bit odd for redis to make adjustments in the API according to the version the module expects. @yossigo WDYT?
Comment From: yossigo
@oranagra I agree, and there's a price to pay for maintaining strict backwards compatibility.