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

String handling fixes and simplifications #49

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

Conversation

jzakrzew
Copy link

While working on my previous PR (#48) I ran into issues with string handling that would cause ruby to crash with a segfault. This PR aims to fix most (but not all) of those problems.

  • Refactor most of string handling code to use rb_str_* functions. This fixes numerous instances of unsafe string handling using strcpy/sprintf that could lead to buffer overflows and simplifies code in other places.
  • Don't copy ruby strings to stack buffers when not needed, just use STR2CSTR (requires RB_GC_GUARD). This fixes possible stack buffer overflows.
  • Change definition of STR2CSTR to StringValueCStr, as StringValuePtr does not even guarantee returning a null-terminated string.

Things that are not fixed:

  • rb_gsl_complex_printf (will do another PR, because I don't see an obvious way to fix it without possibly breaking compatibility)
  • any function that calls popen (I find these very suspect and would need more time to think them through).

* Refactor most of string handling code to use rb_str_* functions. This fixes numerous instances of unsafe string handling using strcpy/sprintf that could lead to buffer overflows and simplifies code in other places.
* Don't copy ruby strings to stack buffers when not needed, just use STR2CSTR (requires RB_GC_GUARD). This fixes possible stack buffer overflows.
* Change definition of STR2CSTR to StringValueCStr, as StringValuePtr does not even guarantee returning a null-terminated string.
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