adding decompress to free(state) before return (targeting Lunar)#1313
adding decompress to free(state) before return (targeting Lunar)#1313dirk-thomas merged 3 commits intolunar-develfrom
Conversation
| ret = roslz4_compressStart(&stream, block_size_id); | ||
| if (ret != ROSLZ4_OK) { return ret; } | ||
| if (ret != ROSLZ4_OK) { | ||
| roslz4_compressEnd(&stream); |
There was a problem hiding this comment.
It seems kind of weird to call compressEnd when compressStart is the one that is failing. I think it might make more sense to fix roslz4_compressStart so that if the call to streamResizeBuffer fails, it calls streamStateFree and then returns the error. What do you think?
There was a problem hiding this comment.
Looking at the current implementation of these functions the patch looks good to me (compressStart is doing two things which can individually fail). Maybe you can clarify what you think can go wrong with this change?
There was a problem hiding this comment.
This change will work fine, but I'm suggesting that the contract with roslz4_compressStart should be that it either succeeds with some memory allocated, or should fail with no visible side-effects. That would mean that if something failed internally, it would clean up after itself and not depend on the caller to cleanup for it. This would also make it easier for any future user of the API to use it, since they wouldn't have to remember to cleanup on failure.
There was a problem hiding this comment.
I will consider to merge the current patch as-is. Please feel free to refactor the current API / implementation in a separate PR.
There was a problem hiding this comment.
Sure, that sounds fine.
utilities/roslz4/src/lz4s.c
Outdated
| ret = roslz4_decompressStart(&stream); | ||
| if (ret != ROSLZ4_OK) { return ret; } | ||
| if (ret != ROSLZ4_OK) { | ||
| roslz4_decompressEnd(&stream); |
There was a problem hiding this comment.
Same idea here, though in this case, I don't think that roslz4_decompressStart can leak memory on an allocation failure, so we might not want to change this at all.
There was a problem hiding this comment.
I agree with you that this shouldn't be necessary since roslz4_decompressStart only allocates when returning OK. So in case of it failing it doesn't need to deallocate anything.
* adding decompress to free(state) before return * copy paste typo error on roslz4_decompressEnd * revert unnecessary change
Replaces #1293 targeting the latest development branch.