Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: usr: clean up memory leaks found with valgrind #54

Merged
merged 2 commits into from
Aug 30, 2023
Merged

Conversation

SJLC
Copy link
Member

@SJLC SJLC commented Aug 26, 2023

Get the test apps to pass 'valgrind --leak-check=full' without any leaks detected.

  • redis_ipc.c - add some missing frees

  • json.hh - fix reference management

    • get_field(): the json constructor of return value will automatically take a reference on the wrapped json_object, so don't need to manually bump reference count with json_object_get()
    • set_field() with json value: do manually take reference so the value object passed in will not be destroyed when the newly assigned parent object gets destroyed, since "add" does not bump reference count but later parent will try to clean it up

Get the test apps to pass 'valgrind --leak-check=full' without
any leaks detected.

* redis_ipc.c - add some missing frees

* json.hh - fix reference management
  * get_field(): the json constructor of return value will automatically
    take a reference on the wrapped json_object, so don't need to manually
    bump reference count with json_object_get()
  * set_field() with json value: *do* manually take reference so the value
    object passed in will not be destroyed when the newly assigned
    parent object gets destroyed, since "add" does not bump reference
    count but later parent will try to clean it up
@SJLC
Copy link
Member Author

SJLC commented Aug 30, 2023

This is now non-WIP, think I've got the JSON reference counting nailed down now.

* switch back to *not* take an extra reference on json_object ptr
  that gets passed in, to make things convenient for typical external
  usage of this constructor. This makes behavior match the exception
  mentioned in original comment about constructors:
    // normally want to take reference on underlying json_object*,
    // except for case of initializing from an existing raw json_object*
    // such as those returned by redis_ipc -- those start out with a reference

  which means that callers no longer need to remember to call json_object_put()
  after wrapping a raw json_object with json class

* add a new comment right above tha constructor highlighting that
  this constructor wrapping an existing json_object does *not* bump
  the reference count

* fix memory leak in operator=() by forgetting to json_object_put()
  before overwriting with a new value
@github-actions
Copy link

Package Line Rate Branch Rate Complexity Health
inc 45% 53% 0
src 65% 45% 0
Summary 62% (662 / 1076) 46% (147 / 320) 0

@github-actions
Copy link

Hello @SJLC! Thanks for opening this PR. We found the following information based on analysis of the coverage report:

Base Branch Rate coverage is 46%
Merging f9a2eea into develop will not change coverage

Nice work, @SJLC. Cheers! 🚀

@SJLC SJLC merged commit 2a36b82 into develop Aug 30, 2023
22 checks passed
@SJLC SJLC deleted the valgrind-fixes branch August 30, 2023 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant