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 segfault when memmoving with negative/enormous n #21

Merged
merged 1 commit into from
Mar 5, 2016

Conversation

jnpkrn
Copy link
Contributor

@jnpkrn jnpkrn commented Mar 1, 2016

I observed a segmentation fault caused by trying to memmove by -15,
which makes 18446744073709551601 on my 64-bit platform after an argument
type promotion (from int into size_t). In my case, this was connected
with filling up disk during the test facilitated by check, hence I derive
that the main issue was that not enough bytes for particular type of
message was actually read (and previously written, for that matter) and
because of this incompleteness, get_result happily consumed more bytes
than was read.

Additional debugging info at the point of segfault (src/check_pack.c):

468│         /* Move remaining data in buffer to the beginning */
469├>        memmove(buf, buf + n, nparse);
470│         /* If EOF has not been seen */
471│         if(nread > 0)
(gdb) p nparse
$1 = -15
(gdb) p n
$2 = 23
(gdb) p nread
$3 = 0

I observed a segmentation fault caused by trying to memmove by -15,
which makes 18446744073709551601 on my 64-bit platform after an argument
type promotion (from int into size_t).  In my case, this was connected
with filling up disk during the test facilitated by check, hence I derive
that the main issue was that not enough bytes for particular type of
message was actually read (and previously written, for that matter) and
because of this incompleteness, get_result happily consumed more bytes
than was read.

Additional debugging info at the point of segfault (src/check_pack.c):

> 468│         /* Move remaining data in buffer to the beginning */
> 469├>        memmove(buf, buf + n, nparse);
> 470│         /* If EOF has not been seen */
> 471│         if(nread > 0)
>
> (gdb) p nparse
> $1 = -15
> (gdb) p	n
> $2 = 23
> (gdb) p	nread
> $3 = 0
@jnpkrn jnpkrn force-pushed the fix-memmove-segfault branch from 6abb2ce to 25a8706 Compare March 1, 2016 19:30
@jnpkrn
Copy link
Contributor Author

jnpkrn commented Mar 2, 2016

Note that

get_result happily consumed more bytes than was read

is just as serious, however I don't feel competent to do such a complex
refactoring, i.e., propagation of the number of bytes that can still be
consumed before reading pass the defined content within the buffer,
on my own.

@brarcher
Copy link
Contributor

brarcher commented Mar 2, 2016

Jenkins: ok to test

brarcher added a commit that referenced this pull request Mar 5, 2016
Fix segfault when memmoving with negative/enormous n
@brarcher brarcher merged commit 71034f6 into libcheck:master Mar 5, 2016
@brarcher
Copy link
Contributor

brarcher commented Mar 5, 2016

Looking at the code, I agree that get_result should not attempt to consume more than is available. Probably there is a more graceful way to handle such a failure. Though, likely all it would do is attempt to abort because the framework hit a condition it is unable to handle.

Thanks for the change. If you could, may you also submit a pull request adding yourself to the AUTHORS file if you are not already listed?

@jnpkrn jnpkrn deleted the fix-memmove-segfault branch March 7, 2016 09:51
@jnpkrn
Copy link
Contributor Author

jnpkrn commented Mar 7, 2016

Done: PR #22.

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