BBC: Math-FastGF2-0.07 triggers "stack smashing detected" #19983
Comments
jkeenan
added
the
BBC
This is strange. I was able to reproduce this "stack smashing" on Linux at a commit more recent than the one @andk cites. However, I was not able to reproduce these failures on a recently built perl on FreeBSD. Could this problem be platform-specific? @khwilliamson, can you take a look? Thank you very much. |
FWIW, I see the problem on Windows (MSWin32-x64-multi-thread), perl-5.37.2 It manifests itself as a couple of pop-ups telling me that perl.exe has stopped working. All tests pass when I build the Math-FastGF2-0.07 with perl-5.36.0. There's a few warnings issued during make: That copy'n'paste is from the 5.37.2 build. I think it's pretty much the same warnings (though not identical) on the 5.36.0 build. Cheers, |
|
I find (on Windows) that the problem goes away if, in perlsubs.c (which is part of the Math-FastGF2-0.07 source distro), I replace each occurrence of This enables the module to build and test successfully on both perl-5.36.0 and perl-5.37.2 - and it also removes the vast majority of the warnings that were being emitted. Does that patch look like a correct fix ? Cheers, |
|
SvPV (and similar) are documented that 'len' is to be STRLEN. This module declares it to be int. That was harmless before the blamed commit, but with it, it results in undefined behavior. I can think of several options. If there is only a little bit of code that calls one of these with an improper len, then they can be fixed. If there are a bunch (which I and #irc thing is likely), then something needs to change. We can determine this by adding an assert. I think this should be the first step. The commit could be reverted which would reintroduce the long-standing behavior of 'sv' being evaluated more than once, which the commit was attempting to fix. The commit would be changed so that if the system had GCC brace groups, it would work properly. And if the system didn't, the per-thread variables PL_Sv and PL_na could be used, with some loss of efficiency, to achieve the single evaluation goal. Opinions welcome |
This is to see how many CPAN modules call these with a differently sized 'len' than the specified 'STRLEN'. See #19983
|
See #19990 |
The memory is allocated by |
|
Also affected: MBAILEY/Digest-OAT-0.04.tar.gz |
As was the case in my comment of 3 days ago, I was able to reproduce this test failure on Linux but not on FreeBSD. So there is an OS-specific aspect to this problem. |
|
On 7/25/22 12:22, James E Keenan wrote:
Also affected: MBAILEY/Digest-OAT-0.04.tar.gz Sample fail report:
http://www.cpantesters.org/cpan/report/566f2156-0c40-11ed-8ab5-218bd7b38173
<http://www.cpantesters.org/cpan/report/566f2156-0c40-11ed-8ab5-218bd7b38173>
As was the case in my comment
<#19983 (comment)> of
3 days ago, I was able to reproduce this test failure on Linux but *not*
on FreeBSD. So there is an OS-specific aspect to this problem.
It's a hardare/configuration issue. If the size of int and the size of
STRLEN, the problem won't manifest itself (in the GF2 case; I haven't
looked at the new one)
… |
Additional data: On FreeBSD-12, where I normally compile with So this maybe an "OS + C-compiler" issue. |
The result of the bug (which the C compiler warns about) in Math-FastGF2 is undefined behaviour - this means the result can be anything, the code might execute as the author expected, or it might smash the stack, or it might whistle a tune. It's a long standing bug in Math-GF2 which the compiler has been warning about all along. You listed the test result of Digest::OAT - do you see type mismatch warnings when building it? |
|
I see the same issue - there's just one instance of an UPDATE: Here's the actual patch I used: Cheers, |
Re-doing the work done in the |
|
Also affected: SERG/Lingua-RU-Translit-0.02.tar.gz |
This behaves similar to Digest::OAT, i.e., on FreeBSD-12 it installs successfully against a blead perl compiled with clang10 (albeit with a lot of build-time warnings), but segfaults when that perl is compiled with gcc10. |
That's a yes. From
From Same bug. |
|
Also affected: AUDREYT/Locale-Hebrew-1.05.tar.gz |
|
Also affected: IWOODHEAD/Bloom16-0.01.tar.gz |
|
Given the amount of CPAN breakage that has been attributed to 1ef9039, I think it's time for us to consider reverting this commit. @khwilliamson, can you investigate? |
This should fix GH Perl#19983. Some of the macros that extract a PV from an SV also will set a 'len' parameter to how many bytes long it is. The len parameter is supposed to be declared as a STRLEN (or equivalently, Size_t). But there is a significant amount of code that declares the parameter wrongly, such as an int, and this code generally has worked. I do believe that warnings are generated. With 1ef9039 such code broke. One could view this as similar to the hash key retrieval order problem from years past, where we viewed the breakage as a "good thing" to catch real bugs early. But in this case, an int may be large enough so that the issue wouldn't ever arise in practice. What this commit does is to see if the 'len' parameter is the same size and sign as STRLEN. If so, it follows the code in 1ef9039. I believe this is technically undefined behavior, as the only defined behavior is if the pointers point to the same object type, but we do such things all the time without negative consequences. If 'len' isn't equivalent to STRLEN, the implementation falls back to using gcc brace groups, when available, to only evaluate the passed in SV once. If not available, it uses temporary variables for the same effect.
This should fix GH Perl#19983. Some of the macros that extract a PV from an SV also will set a 'len' parameter to how many bytes long it is. The len parameter is supposed to be declared as a STRLEN (or equivalently, Size_t). But there is a significant amount of code that declares the parameter wrongly, such as an int, and this code generally has worked. I do believe that warnings are generated. With 1ef9039 such code broke. One could view this as similar to the hash key retrieval order problem from years past, where we viewed the breakage as a "good thing" to catch real bugs early. But in this case, an int may be large enough so that the issue wouldn't ever arise in practice. What this commit does is to see if the 'len' parameter is the same size and sign as STRLEN. If so, it follows the code in 1ef9039. I believe this is technically undefined behavior, as the only defined behavior is if the pointers point to the same object type, but we do such things all the time without negative consequences. If 'len' isn't equivalent to STRLEN, the implementation falls back to using gcc brace groups, when available, to only evaluate the passed in SV once. If not available, it uses temporary variables for the same effect.
This should fix GH Perl#19983. Some of the macros that extract a PV from an SV also will set a 'len' parameter to how many bytes long it is. The len parameter is supposed to be declared as a STRLEN (or equivalently, Size_t). But there is a significant amount of code that declares the parameter wrongly, such as an int, and this code generally has worked. I do believe that warnings are generated. With 1ef9039 such code broke. One could view this as similar to the hash key retrieval order problem from years past, where we viewed the breakage as a "good thing" to catch real bugs early. But in this case, an int may be large enough so that the issue wouldn't ever arise in practice. What this commit does is to see if the 'len' parameter is the same size and sign as STRLEN. If so, it follows the code in 1ef9039. I believe this is technically undefined behavior, as the only defined behavior is if the pointers point to the same object type, but we do such things all the time without negative consequences. If 'len' isn't equivalent to STRLEN, the implementation falls back to using gcc brace groups, when available, to only evaluate the passed in SV once. If not available, it uses temporary variables for the same effect.
This should fix GH Perl#19983. Some of the macros that extract a PV from an SV also will set a 'len' parameter to how many bytes long it is. The len parameter is supposed to be declared as a STRLEN (or equivalently, Size_t). But there is a significant amount of code that declares the parameter wrongly, such as an int, and this code generally has worked. I do believe that warnings are generated. With 1ef9039 such code broke. One could view this as similar to the hash key retrieval order problem from years past, where we viewed the breakage as a "good thing" to catch real bugs early. But in this case, an int may be large enough so that the issue wouldn't ever arise in practice. What this commit does is to see if the 'len' parameter is the same size and sign as STRLEN. If so, it follows the code in 1ef9039. I believe this is technically undefined behavior, as the only defined behavior is if the pointers point to the same object type, but we do such things all the time without negative consequences. If 'len' isn't equivalent to STRLEN, the implementation falls back to using gcc brace groups, when available, to only evaluate the passed in SV once. If not available, it uses temporary variables for the same effect.
This should fix GH Perl#19983. Some of the macros that extract a PV from an SV also will set a 'len' parameter to how many bytes long it is. The len parameter is supposed to be declared as a STRLEN (or equivalently, Size_t). But there is a significant amount of code that declares the parameter wrongly, such as an int, and this code generally has worked. I do believe that warnings are generated. With 1ef9039 such code broke. One could view this as similar to the hash key retrieval order problem from years past, where we viewed the breakage as a "good thing" to catch real bugs early. But in this case, an int may be large enough so that the issue wouldn't ever arise in practice. What this commit does is to see if the 'len' parameter is the same size and sign as STRLEN. If so, it follows the code in 1ef9039. I believe this is technically undefined behavior, as the only defined behavior is if the pointers point to the same object type, but we do such things all the time without negative consequences. If 'len' isn't equivalent to STRLEN, the implementation falls back to using gcc brace groups, when available, to only evaluate the passed in SV once. If not available, it uses temporary variables for the same effect.
This is to see how many CPAN modules call these with a differently sized 'len' than the specified 'STRLEN'. See Perl#19983
This should fix GH Perl#19983. Some of the macros that extract a PV from an SV also will set a 'len' parameter to how many bytes long it is. The len parameter is supposed to be declared as a STRLEN (or equivalently, Size_t). But there is a significant amount of code that declares the parameter wrongly, such as an int, and this code generally has worked. I do believe that warnings are generated. With 1ef9039 such code broke. One could view this as similar to the hash key retrieval order problem from years past, where we viewed the breakage as a "good thing" to catch real bugs early. But in this case, an int may be large enough so that the issue wouldn't ever arise in practice. What this commit does is to see if the 'len' parameter is the same size and sign as STRLEN. If so, it follows the code in 1ef9039. I believe this is technically undefined behavior, as the only defined behavior is if the pointers point to the same object type, but we do such things all the time without negative consequences. If 'len' isn't equivalent to STRLEN, the implementation falls back to using gcc brace groups, when available, to only evaluate the passed in SV once. If not available, it uses temporary variables for the same effect.
|
I can't reproduce this issue on macOS. Has it been fixed? |
|
On Linux, I am still getting test failures in several of the distros cited in this ticket. From my (Bloom16 has prerequisite problems that prevented me from getting far enough to see whether it was failing due to the problems raised in this ticket.) |
It's a bug in the downstream modules, which C compilers warn about in older perls (and does for each of these). I doubt some of the modules will be fixed, since they haven't had anything like a recent release: Digest::OAT: last release 2009 (Math::FastGF2 2019, so it might get a release) I've added comments to the ticket for each of these (and created a ticket for Bloom16) describing the cause of the problem, quoting at least one example of the problem in that module and the warning from the compiler testing under my system perl 5.32.1. |
|
I'm not sure I think this is a blocker. This is sort of the low key breakage we expect once in a while when Perl changes its code, assuming people are doing things as specified. @khwilliamson Thoughts? |
|
See #20037 (comment) That PR could be merged, but there is unease about doing so. As @tonycoz pointed out, the module authors ignored compiler warnings. |
Description
With 1ef9039 the tests for DMALONE/Math-FastGF2/Math-FastGF2-0.07.tar.gz were starting to fail. Sample test report with v5.37.2: http://www.cpantesters.org/cpan/report/21cf2db8-0954-11ed-aef9-e4323abf57b8
Steps to Reproduce
cpan -i DMALONE/Math-FastGF2/Math-FastGF2-0.07.tar.gz
Expected behavior
Tests should succeed and module should be installed.
Observed behavior
Perl configuration
The text was updated successfully, but these errors were encountered: