I was making a module and one of my unit tests was failing in the case where I was passing 0 as the newlen argument to RedisModule_StringTruncate.
I think the issue is here:
https://github.com/antirez/redis/blob/unstable/src/module.c#L1453
When newlen is 0, this passes -1 to sdsrange, which maintains the whole string.
I was starting to make a pull request, but thought perhaps my understanding of the semantics was wrong, i.e. does newlen include a NULL character and 0 is an invalid value to pass?
Comment From: itamarhaber
While that is the expected behavior from the underlying sdsrange function, I do not think that this was the intended effect for StringTruncate with 0 newlen so a PR make sense IMO.
Comment From: neomantra
Any update on including this issue / PR? I see that the RM_StringTruncate code hasn't changed since this PR so it should merge smoothly.
It also effectively just adds these lines, although the whitespace diff makes it seem like more:
if (newlen == 0) {
sdsfree(key->value->ptr);
key->value->ptr = sdsempty();
} else {
// previous sdsrange code
Right now, if the value of key was a 1024-byte string, RM_StringTruncate(key, 1) would result in a 1-byte string value, but RM_StringTruncate(key,0) results in the same 1024-byte string value. I don't think that's expected.
I couldn't find test tooling for the module APIs? If there is I'd be happy to make a test case.
Comment From: neomantra
Can this PR be reviewed and included in Redis 5.0?
The relevant RM_StringTruncate function is unchanged, but is now at:
https://github.com/antirez/redis/blob/unstable/src/module.c#L1602
It is a real bug and I've had to work around it in a couple modules I've created over the last year. I can rebase the pull request, or feel free to just make the needed change. Thanks!
Comment From: neomantra
I missed that there was a test module so I have added testing for RM_StringTruncate to it. I also fixed a usability issue with the test assertion functions. I also rebased the pull request to the latest.
As noted, the test fails without the newlen == 0 handling. Hopefully this makes it more clear that this is a real bug and that this pull request fixes it. Let me know of anything further I can do. Thanks!
Comment From: neomantra
@itamarhaber do you have repo permissions to label this as a confirmed bug? I beefed up the unit tests to clearly show that and it's also evident from inspection -- which I think you agreed with in the PR?
I'm not sure what else to do to promote the inclusion of this pull request in Redis. Internally, we've blacklisted this RedisModule_StringTruncate call because it is broken.
Comment From: itamarhaber
@neomantra I do not have permissions, but it looks like a bug to me :) @antirez?
Comment From: neomantra
I wanted to note that, after being away from this issue for a couple years, I reworked it to have a simpler change:
sdsrange(key->value->ptr,0,(newlen == 0) ? 0 : newlen-1);
I thought it was working, but then remembered that the modules didn't get tested via make (except now they do with make test-modules in #3718). Then I re-realized that sdsrange can't make empty strings (unless the string started out empty). Putting this all here for my future self.
Comment From: neomantra
@oranagra Thanks for looking at this and finally committing a fix to this issue!
Seems like there isn't anything for me to in this issue any more, but if there is, just let me know.
Comment From: oranagra
@neomantra thanks for reporting (and fixing), and sorry for the huge delay. when l looked at #3718 for some reason i assumed you're no longer following that PR issue (although now i see you did recently comment there), so instead of delaying this PR further more, i just took matters into my own hands. if the changes i added to your PR look good to you (and maybe the followup #9125), then there's nothing more to do. either way, this issue can now be closed.