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 error handling #37

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Fix error handling #37

merged 1 commit into from
Feb 6, 2024

Conversation

krzykro2
Copy link
Contributor

@krzykro2 krzykro2 commented Feb 4, 2024

In current code the vtable of StreamErrorHandler gets stored and accessed correctly - that's why simple structs with trait implementations that don't read self work well. However, somewhere inside the entrails of screencapturekit-sys the original struct gets dropped (but surprisingly not its vtable! you can verify this by adding custom Drop impl and seeing it triggered, while the vtable still routes on_error calls correctly). This leads to segfaults (because due to vtable being stored correctly, the code tries to access already freed data, which leads to bad data access and sometimes segfaults).

I tried taking ownership of the Handler somewhere inside the safe part of the library, but that involved a lot of lifetime annotations. It's still something we might want to do, because currently once the Handler gets added - it will never be freed and it will live inside the global HashMap forever. And sadly even when I got it to work, the original struct did not get dropped, but the address to self inside the on_error callback was still wrong - most likely some pointer arithmetic issue (maybe due to the fact that dyn Trait is actually a double pointer?).

I added a simple test to showcase the source of the issue. Without my change the error_rx.recv() would return an Error claiming that error_tx has already been dropped. What's worse is that self.error_tx.send(()) would result in segfaults.

My next step might be actually passing the error given by SCK back to the handler (which currently takes no arguments).

Thanks for having a look.

@1313
Copy link
Collaborator

1313 commented Feb 5, 2024

I will have a look, thanks for investigating. I am in the process of doing a semi rewrite in pr: #27.

This issue probably exist there as well so good to verify and fix.

I'll get back to this later today.

Cheers and thanks again

@krzykro2
Copy link
Contributor Author

krzykro2 commented Feb 6, 2024

Thanks for doing the rewrite to use the better library.

A proposed course of action:

  • If you think this PR is safe, we could merge it into main and release as 0.2.6 so that myself and other consumers of this library can have a working error handler (otherwise I will have to build my product against my own fork of the library which is sub-optimal).
  • You add the test case I defined to your new PR and make sure it passes with your implementation (I had a quick look and it seems like the pointer arithmetic is the same as in main so it might have the same problem)

Let me know what you think.

@1313
Copy link
Collaborator

1313 commented Feb 6, 2024

Sounds good 👍

@1313 1313 merged commit 841193b into doom-fish:main Feb 6, 2024
1 check passed
@1313
Copy link
Collaborator

1313 commented Feb 6, 2024

Pushed a new version. 0.2.6.

The CF version will be using version 0.3 and onwards FYI.

1313 added a commit that referenced this pull request Feb 16, 2024
* main:
  v0.2.6
  Update rust.yml
  chore: fix warnings
  fix error handling (#37)
  cargo fmt (#38)
  docs: add krzykro2 as a contributor for code (#36)
  make safe StreamConfiguration defaults equal to unsafe (#35)
  Revert "test ci"
  test ci
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