rails-sqlserver / tiny_tds

TinyTDS - Simple and fast FreeTDS bindings for Ruby using DB-Library.
Other
605 stars 191 forks source link

Fix dangling pointer after client.close #527

Closed matias-martini closed 1 year ago

matias-martini commented 1 year ago

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

aharpervc commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

aharpervc commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

matias-martini commented 1 year ago

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

Thanks again for taking the time to review the changes!