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 buffer overrun issue due to strncat usage #8

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@daleevans
Contributor

daleevans commented May 25, 2015

strncat(a,b,n) can write up to n+1 bytes. Allocates an extra byte so that the trailing null will have somewhere to go in the case where we have more than ZZLEXBUFFERSIZE bytes of data to deal with. Also contains test code.

daleevans added some commits May 25, 2015

@ambs

This comment has been minimized.

Show comment
Hide comment
@ambs

ambs May 27, 2015

Owner

Seems good. Will merge later today, after testing in some different platforms. Thanks.

Owner

ambs commented May 27, 2015

Seems good. Will merge later today, after testing in some different platforms. Thanks.

@daleevans

This comment has been minimized.

Show comment
Hide comment
@daleevans

daleevans May 27, 2015

Contributor

Oh, in case you were wondering what environment I replicated the crash in, it was an AWS EC2 instance running debian-squeeze-amd64-pvm-2014-07-21-ebs (ami-01146c31).

Contributor

daleevans commented May 27, 2015

Oh, in case you were wondering what environment I replicated the crash in, it was an AWS EC2 instance running debian-squeeze-amd64-pvm-2014-07-21-ebs (ami-01146c31).

@ambs

This comment has been minimized.

Show comment
Hide comment
@ambs

ambs May 27, 2015

Owner

God :-)

Owner

ambs commented May 27, 2015

God :-)

@@ -0,0 +1,2 @@
@comment{{7890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567abc-this-is-way-oveaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaar }

This comment has been minimized.

@ambs

ambs May 27, 2015

Owner

Just to confirm the idea, isn't a '}' missing at the end? -- given it starts with two {{

@ambs

ambs May 27, 2015

Owner

Just to confirm the idea, isn't a '}' missing at the end? -- given it starts with two {{

This comment has been minimized.

@ambs

ambs May 27, 2015

Owner

(no need to fix it, I'll fix locally if needed)

@ambs

ambs May 27, 2015

Owner

(no need to fix it, I'll fix locally if needed)

@daleevans

This comment has been minimized.

Show comment
Hide comment
@daleevans

daleevans May 27, 2015

Contributor

Yes, that's the bug -- when the code detects a malformed comment it tries to alert the user, but if the malformed comment contains more than 2000 bytes of data it blows up by writing a NULL just past the allocated buffer space. If it weren't malformed the reallocation code would deal with it correctly, it's only zzFAIL() that has the issue.

Contributor

daleevans commented May 27, 2015

Yes, that's the bug -- when the code detects a malformed comment it tries to alert the user, but if the malformed comment contains more than 2000 bytes of data it blows up by writing a NULL just past the allocated buffer space. If it weren't malformed the reallocation code would deal with it correctly, it's only zzFAIL() that has the issue.

@ambs

This comment has been minimized.

Show comment
Hide comment
@ambs

ambs May 27, 2015

Owner

but I am getting this:

t/overflow.t .... 1/3 btparse/tests/data/overflow.bib, line 3, syntax error: at end of input, expected one of: start of entry ("{" or "(") or quoted string ({...} or "...")
t/overflow.t .... Failed 1/3 subtests
t/parse.t ....... ok

So, it seems it is not passing yet...

Owner

ambs commented May 27, 2015

but I am getting this:

t/overflow.t .... 1/3 btparse/tests/data/overflow.bib, line 3, syntax error: at end of input, expected one of: start of entry ("{" or "(") or quoted string ({...} or "...")
t/overflow.t .... Failed 1/3 subtests
t/parse.t ....... ok

So, it seems it is not passing yet...

@daleevans

This comment has been minimized.

Show comment
Hide comment
@daleevans

daleevans May 27, 2015

Contributor

Hmm,

I get an error when I run

prove --lib lib t/overflow.t

but I don't when I run

./Build test

The difference between the two appears to be PERL_DL_NONLAZY=1 set in the environment when I run Build but not when I run prove. If instead I run

PERL_DL_NONLAZY=1 prove --verbose --lib lib t/overflow.t

It works. It seems like there is some issue in the XS layer, possibly handling the result of an incomplete parse. I have this stack trace:

Program terminated with signal 11, Segmentation fault.
#0 0x00007f57e540e838 in convert_value_entry (preserve=0, entry=0x10169d8, top=0x1228e80) at xscode/btxs_support.c:286
286 hv_store (lines, "STOP", 4, newSViv (last_line), 0);
(gdb) where
#0 0x00007f57e540e838 in convert_value_entry (preserve=0, entry=0x10169d8, top=0x1228e80) at xscode/btxs_support.c:286
#1 ast_to_hash (entry_ref=entry_ref@entry=0x10bdfe0, top=0x1228e80, parse_status=0, preserve=preserve@entry=0) at xscode/btxs_support.c:401
#2 0x00007f57e54112f1 in XS_Text__BibTeX__Entry__parse (my_perl=, cv=) at xscode/BibTeX.xs:196
#3 0x00007f57e693f71c in Perl_pp_entersub () from /usr/lib/libperl.so.5.14
#4 0x00007f57e6936cf6 in Perl_runops_standard () from /usr/lib/libperl.so.5.14
#5 0x00007f57e68d8825 in perl_run () from /usr/lib/libperl.so.5.14
#6 0x0000000000400f89 in main ()

I will look into this some more and see if I can figure out what the issue is.

Contributor

daleevans commented May 27, 2015

Hmm,

I get an error when I run

prove --lib lib t/overflow.t

but I don't when I run

./Build test

The difference between the two appears to be PERL_DL_NONLAZY=1 set in the environment when I run Build but not when I run prove. If instead I run

PERL_DL_NONLAZY=1 prove --verbose --lib lib t/overflow.t

It works. It seems like there is some issue in the XS layer, possibly handling the result of an incomplete parse. I have this stack trace:

Program terminated with signal 11, Segmentation fault.
#0 0x00007f57e540e838 in convert_value_entry (preserve=0, entry=0x10169d8, top=0x1228e80) at xscode/btxs_support.c:286
286 hv_store (lines, "STOP", 4, newSViv (last_line), 0);
(gdb) where
#0 0x00007f57e540e838 in convert_value_entry (preserve=0, entry=0x10169d8, top=0x1228e80) at xscode/btxs_support.c:286
#1 ast_to_hash (entry_ref=entry_ref@entry=0x10bdfe0, top=0x1228e80, parse_status=0, preserve=preserve@entry=0) at xscode/btxs_support.c:401
#2 0x00007f57e54112f1 in XS_Text__BibTeX__Entry__parse (my_perl=, cv=) at xscode/BibTeX.xs:196
#3 0x00007f57e693f71c in Perl_pp_entersub () from /usr/lib/libperl.so.5.14
#4 0x00007f57e6936cf6 in Perl_runops_standard () from /usr/lib/libperl.so.5.14
#5 0x00007f57e68d8825 in perl_run () from /usr/lib/libperl.so.5.14
#6 0x0000000000400f89 in main ()

I will look into this some more and see if I can figure out what the issue is.

@ambs

This comment has been minimized.

Show comment
Hide comment
@ambs

ambs May 27, 2015

Owner

For me it fails with

PERL_DL_NONLAZY=1 prove --verbose --lib lib t/overflow.t

(perl 5.20).
Will try to debug it. Was asking before looking to the code as you might have stepped on that.

Owner

ambs commented May 27, 2015

For me it fails with

PERL_DL_NONLAZY=1 prove --verbose --lib lib t/overflow.t

(perl 5.20).
Will try to debug it. Was asking before looking to the code as you might have stepped on that.

@daleevans

This comment has been minimized.

Show comment
Hide comment
@daleevans

daleevans May 28, 2015

Contributor

I believe this fixes the crash in perl, but I'm not totally confident in what the "right" behaviour is when there are no values. Not storing anything will probably be fine, as there's nothing to store?

I also noticed that in convert_assigned_entry, prev_line is similarly uninitialized and may be used uninitialized if bt_next_field never returns anything. Just an integer, probably won't result in a crash, but might result in some incorrect results somewhere.

Contributor

daleevans commented May 28, 2015

I believe this fixes the crash in perl, but I'm not totally confident in what the "right" behaviour is when there are no values. Not storing anything will probably be fine, as there's nothing to store?

I also noticed that in convert_assigned_entry, prev_line is similarly uninitialized and may be used uninitialized if bt_next_field never returns anything. Just an integer, probably won't result in a crash, but might result in some incorrect results somewhere.

@ambs

This comment has been minimized.

Show comment
Hide comment
@ambs

ambs May 28, 2015

Owner

And merged. thank you for the exception work!

Owner

ambs commented May 28, 2015

And merged. thank you for the exception work!

@ambs ambs closed this May 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment