Skip to content

Conversation

@oleg-vinted
Copy link

Pulling from vinted#17:

This piece of code contains a buffer overflow issue because it uses a combination of memcpy and snprintf calls that don't take the remaining buffer capacity into account.

I've used the approach used on line 97 which uses a single snprintf call to make sure we don't overrun the buffer.

Without the patch, CI fails with the following crash:

cp vendor/libmemcached-0.32/tests/udp.c tmp/x86_64-linux/stage/vendor/libmemcached-0.32/tests/udp.c
install -c tmp/x86_64-linux/rlibmemcached/3.0.7/rlibmemcached.so lib/rlibmemcached.so
cp tmp/x86_64-linux/rlibmemcached/3.0.7/rlibmemcached.so tmp/x86_64-linux/stage/lib/rlibmemcached.so
/home/runner/work/memcached/memcached/lib/memcached.rb:26: warning: already initialized constant Memcached::VERSION
/home/runner/work/memcached/memcached/lib/memcached/version.rb:2: warning: previous definition of VERSION was here
memcached(2403): Operation not permitted
/home/runner/work/memcached/memcached/test/unit/rails_test.rb:9: warning: constant Memcached::Rails is deprecated
/home/runner/work/memcached/memcached/test/unit/rails_test.rb:9: warning: constant Memcached::Rails is deprecated
Loaded suite /home/runner/work/memcached/memcached/vendor/bundle/ruby/3.0.0/gems/rake-13.0.3/lib/rake/rake_test_loader
Started
*** buffer overflow detected ***: terminated
Aborted (core dumped)
rake aborted!
Command failed with status (134)
/home/runner/work/memcached/memcached/vendor/bundle/ruby/3.0.0/gems/rake-13.0.3/exe/rake:27:in `<top (required)>'
/opt/hostedtoolcache/Ruby/3.0.7/x64/bin/bundle:23:in `load'
/opt/hostedtoolcache/Ruby/3.0.7/x64/bin/bundle:23:in `<main>'
Tasks: TOP => default => test
(See full trace by running task with --trace)

@oleg-vinted
Copy link
Author

Hello, this PR has been up for a bit, do you accept contributions to this repo? We have our own fork (https://github.com/vinted/memcached/tree/1-0-stable) and I'd like prevent our fork from diverging too much from yours.

(There's one more pending PR: #33.)

Tagging the author of the last commit: @peterzhu2118

@oleg-vinted
Copy link
Author

Tagging more maintainers: @casperisfine

@casperisfine
Copy link

do you accept contributions to this repo?

I guess, but we're no longer using it (or about to stop, can't quite remember).

@casperisfine
Copy link

Code looks fine, but the test suite is hanging on my machine (not just your branch), so it doesn't help with confidently making change to this repo.

This gem is such a tire fire...

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