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

Socket++ fork memory safery improvements #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MadVitaliy
Copy link
Contributor

Right now an order of operations is:

  1. Delete curr.
  2. Access curr in "curr = curr->next"
  3. Check if new curr is nullptr.

curr->next must be preserved before curr is deleted, what I have done in this PR. The flow now is:

  1. If "curr" is nullptr nothing is done, loop is jut skipped (escaped).
  2. "next" is preserved.
  3. "curr" is deleted by the same condition as before, thus the behaviour should not be changed.
  4. The preserve "next" is assigned to "curr"
  5. Begin from 1.

@seanm
Copy link
Contributor

seanm commented Jan 8, 2025

The code change itself lgtm. But...

  1. your explanation write-up is great, but would be nice to have in the actual commit message, not just in the github PR comments. (So that it's visible in git log and git history)
  2. this is 3rd party code so ideally would be fixed upstream. I tried once to find the upstream, but I could not. @malaterre do you know if there is an upstream?

@MadVitaliy
Copy link
Contributor Author

@seanm, I had a look on a possible original upstream. Malaterre's fork of it has a bit newer version than used in GDCM. And it turns out that the newer version has the same fix!

@seanm
Copy link
Contributor

seanm commented Jan 9, 2025

@seanm, I had a look on a possible original upstream. Malaterre's fork of it has a bit newer version than used in GDCM. And it turns out that the newer version has the same fix!

Would you be willing to diff GDCM's copy with that other repo and unify the two? Perhaps they each have fixes missing in the other...

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.

2 participants