MDEV-36929: Warning: Memory not freed: 32 on SELECT COLUMN_JSON()#4734
MDEV-36929: Warning: Memory not freed: 32 on SELECT COLUMN_JSON()#4734abhishek593 wants to merge 1 commit intoMariaDB:10.6from
Conversation
grooverdan
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
This can be fixed in a variety of ways.
One way would be to rely heavily on RAII patterns: have destructors for everything to make sure they are properly called when the PC goes out of scope for these. This will basically fix a class of bugs: past, present and future. At the expense of non-optimal code in certain cases.
Another way would be to hand-craft initialization and de-initialization of data structures at exactly the right time so that memory profiling tools can detect use of uninitialized data and there's no CPU lost in needless double initializing and de-initializing just so that it's "neat". This of course includes awareness of any side effects functions might have on the data structures they operate on.
This review is done in the latter style. As this seems to be the preferred style.
Item_func_dyncol_json::val_str() used an internal DYNAMIC_STRING without ensuring it was freed when mariadb_dyncol_json failed. Fixed by freeing the string in the failure case.
|
@gkodinov I agree with all the points. I didn't look closely on how init_dynamic_string() handled the struct initialization, and that's why I added bzero call to json. I have made the changes suggested by you and also added the test. |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM.
Thanks for working on this with me. Stay tuned for the final review please.
5 participants
Item_func_dyncol_json::val_str() used an internal DYNAMIC_STRING without ensuring it was freed in all execution paths. Fixed by initializing the DYNAMIC_STRING with bzero and calling dynstr_free() on both success and error paths.