Fix dangling pointer after client.close

[matias-martini]

Context

Hey there!

Firstly, I want to express my appreciation for the hard work and dedication that you have put into maintaining this project. Your efforts have been incredibly valuable to me and many others in the community.

Well, as reported in the issue #519, we ran into an unexpected Segmentation Fault Error while verifying whether a client was dead after closing it. Upon investigation, we discovered (following the leads of @aharpervc ) that the root cause of the issue was that by calling dbclose, we were deallocating memory used by the client structure without setting the pointer to NULL after closing the client connection.
This led to a dangling pointer, which ultimately resulted in the Segmentation Fault error when we tried to check whether the client was dead using TDS dbdead (here).

To prevent this issue from happening again, I've made the necessary updates to ensure that the client pointer is set to NULL after every dbclose. This fix will help us avoid any dangling pointers and keep our code running smoothly.

I also tested this locally both before and after building the gem with the changes and everything is working like a charm now!

client = TinyTds::Client.new(**ops)
client.close
client.dead? # --> True

I wanted to note that while another solution would have checked whether the client was closed (cwrap->closed) before calling dbdead, I didn't feel comfortable leaving any dangling pointers. 😅

Let me know if you have any questions or concerns.

Changelog

• Considered testing client.dead? after closing the connection in the Client test cases.
• Fixed dangling pointers after calling dbclose.

[aharpervc]

This looks like a reasonable fix to me. Can you also update the changelog file, by adding a bullet point in the "unreleased" section the way it's been done in the past?

[matias-martini]

Great, I've made the changes you requested! Please take a look and let me know if everything looks good to you. If there's anything else you need me to do or if you have any further suggestions, feel free to let me know! 😬

[matias-martini]

@aharpervc I've just solved some merge conflicts. Let me know I you need me to do something else to merge this PR 🙃

[aharpervc]

I'd like to have our CI jobs run for it, but I'm not seeing that integration show up on this PR. I'm guessing that's due to the "Build forked pull requests" setting being turned off at the moment... I'll turn it on and see what happens.

[aharpervc]

You might need to push to this branch again, can you try that? For example, you can probably squash this branch down to 1 commit & force push.

[matias-martini]

@aharpervc all tests passed! Sorry for the long wait btw, busy month 😅

[aharpervc]

This seems fine to me. I think it's probably best as just 1 commit in the master history, any objections to the "squash & merge" feature?

[andyundso]

Given the context of the issue and the PR description, the changes also look good to me.

@matias-martini nicely applied TDD here (first committing a failing test before committing the fix), so merge-commit and rebase would be good. Still, squashing gives the advantage that you find the fix and the test immediately when browsing the Git history at a later point.

[matias-martini]

@aharpervc @andyundso Totally onboard with 'Squash and merge' for a clearer history! 😄

Thanks again for taking the time to review the changes!