Chunk limit fixes #15

Merged
merged 3 commits into from Jan 4, 2017

Projects

None yet

3 participants

@kivikakk
Collaborator
kivikakk commented Jan 3, 2017 edited

We're getting a lot of segfaults where we try to have more chunks than can be expressed with a uint16_t.

66d939d detects OOM or a chunk overflow condition and raises a Ruby-level exception, instead of silently returning NULL and then causing some segfaults later on.

Note the contract of some functions (mochilo_buf_rechunk, mochilo_buf_rechunk2, init_cur_chunk) has been changed; they either return a valid value, or raise. They never NULL.

15b9ca9 bumps the chunk counter to 32-bits from 16-bits, thus stopping this from occurring in most imaginable cases.

kivikakk added some commits Jan 3, 2017
@kivikakk kivikakk Detect OOM or overflow conditions 66d939d
@kivikakk kivikakk Support more than 32Ki chunks 15b9ca9
@kivikakk
Collaborator
kivikakk commented Jan 3, 2017

The build error is dependency msgpack-0.5.5 failing to build on Ruby HEAD, because it expects rb_cFixnum and rb_cBignum to exist. Tests are all green on 2.0–2.3.

@brianmario

lgtm!

This also kinda makes me want to investigate some runs through AFL...

@brianmario
Owner

As for the msgpack failures - we should probably move that dependency out of the gemspec and into a "benchmark" section of the Gemfile.

ext/mochilo/buffer.c
skip_last_chunk(buf);
if (buf->cur_chunk == buf->chunk_count) {
- buf->chunk_count *= 2;
+ if (!(buf->chunk_count * 2))
@mhp
mhp Jan 3, 2017

This test assumes that buf->chunk_count is a power of two. This is true today, but I don't know if the design requires it forever.

Something like if ( (buf->chunk_count * 2) < buf->chunk_count) might be more robust, if you think it is necessary.

@kivikakk
kivikakk Jan 4, 2017 Collaborator

I like this! Thanks for the idea!

@kivikakk kivikakk Robust vs. non-power-of-2 chunk_count 87c8cdb
@kivikakk
Collaborator
kivikakk commented Jan 4, 2017

@brianmario; if this is looking all good to you, I'd like to release a new version! Would you be able to do that? (Alternatively, if you're happy to give me write access, I'm happy to merge and bump.)

@brianmario
Owner

@kivikakk access granted. Hit me up in slack about which email you use for rubygems!

@kivikakk
Collaborator
kivikakk commented Jan 4, 2017

@brianmario thanks, will do!

@kivikakk kivikakk merged commit d7f4528 into brianmario:v1-release Jan 4, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@kivikakk kivikakk deleted the unknown repository branch Jan 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment