Red Hat Bugzilla – Bug 850911
g++: operator new overflow fix
Last modified: 2014-02-05 07:05:53 EST
Created attachment 606350 [details]
I have backported a fix for an integer overflow in g++-generated code (which can lead to heap-based buffer overflows in some cases). The changes involved adjustment because of the fix for PR 51213 ("access control under SFINAE"), which changed some function argument lists. Initial testing looks good.
The first part, rejecting variably-modified types in operator new, removes an accidental GCC extension. This could break some existing sources, but this extension is not implemented anywhere else, so it should not be present in code which has been compiled with non-GCC compilers in the past.
This change affects g++-generated code, so the usual suspects (LibreOffice, xpdf/poppler, Firefox and other Mozilla applications) should be recompiled with a fixed to compiler, to make the compiler change effective.
There is one outstanding change currently under review, <http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01416.html>, but to my knowledge, this code in libsupc++ is dead as far as GCC is concerned.
Created attachment 608498 [details]
Another backport proposal, based on discussions with Jakub. This one does not reject any previously accepted code. However, it fails to check the array-of-VLA case and issues a warning.
This patch needs further testing, I'm still working on that.
Created attachment 608499 [details]
Diff to previous patch
This shows the differences to the previous patch (which was very close to what was in GCC trunk).
Created attachment 618101 [details]
I don't like either of these patches, as said earlier, introducing new warnings/errors that are enabled by default or by preexisting warning options into a stable GCC release branch is a serious problem.
Exception can be perhaps warnings/errors where there is compile time provable overflow, but definitely not using G++ extensions that were accepted before.
With this untested patch I'm trying to not introduce new warnings for VLAs in new, but use otherwise code pretty similar to what the trunk does (to improve maintainability). If all dimensions (except the outer_nelts) are constants, it should behave the same way, if some of the dimensions are dynamic, it computes separately the constant factor and dynamic factor (inner_nelts), and in the end divides the constant max bound by inner_nelts ?: 1.
If the new warning/error doesn't have false positives, then it's clearly the right thing to do as it improves the overall quality of our system, potentially catching security issues earlier and avoiding the need for disruptive z-stream updates.
Even if it doesn't have false positives, it breaks -Werror builds, which is undesirable in stable release (it is enough that we ask users to do porting to newer major GCC versions, we shouldn't ask them to port their code from gcc-4.7.2-2 to gcc-4.7.2-3).
In this case, the warnings/errors are about G++ VLA extensions, so I don't see how removing that extension mid release improves code quality. This is similar thing to why it is usually undesirable to backport accepts-invalid bugfixes.
Created attachment 618142 [details]
One of the VLA testcases I used for testing this (adapted init/new39.C for VLAs).
Created attachment 618143 [details]
And another testcase (new39a.C is to catch the overflow exceptions, this one to test some smaller sizes that must be accepted).
I haven't read the patch. My understanding was that it introduced new warnings/errors for cases where the allocation could overflow. If it does more than that (ie, removes a feature like VLA extensions), then it's obviously not appropriate.
If all it does is issue warnings/errors for code that has the overflow vulnerability, then it seems appropriate to me.
(In reply to comment #3)
> With this untested patch I'm trying to not introduce new warnings for VLAs
> in new, but use otherwise code pretty similar to what the trunk does (to
> improve maintainability). If all dimensions (except the outer_nelts) are
> constants, it should behave the same way, if some of the dimensions are
> dynamic, it computes separately the constant factor and dynamic factor
> (inner_nelts), and in the end divides the constant max bound by inner_nelts
> ?: 1.
This looks pretty good, it's less involved than I expected. But this multiplication also needs an overflow check:
+ inner_nelts = cp_build_binary_op (input_location,
+ MULT_EXPR, inner_nelts,
+ this_inner_nelts, complain);
I think it should be possible an array of factors and divide the limit by the array elements. That avoids the need for a saturating multiplication, which is complicated because of some sizetype/size_type_node weirdness related to TYPE_MAX_VALUE.
We don't do overflow checking for int a[b][c][d]; either, and two or more dimensional VLAs are really rare, so I think we don't need to worry about that.
(In reply to comment #10)
> We don't do overflow checking for int a[b][c][d]; either, and two or more
> dimensional VLAs are really rare, so I think we don't need to worry about
Okay, I suppose the remaining loopholes are acceptable. The worst offenders in terms of operator new overflows have been compiled with other compilers as well, so they don't make use of such GCC conformance bugs.
By the way, it's not just VLAs, we also accept new int[a][b] in templates if b is a template-parameter-dependent expression which does not expand to a constant in an instantiation. (GCC rejects only known variables at parse time.) So some of the new tests need adjusting.
The https://bugzilla.redhat.com/show_bug.cgi?id=850911#c3 patch is now in gcc-4.7.2-3.fc19. I can build it into F18 too if it works well for you in F19.
This message is a reminder that Fedora 18 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 18. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as WONTFIX if it remains open with a Fedora
'version' of '18'.
Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version prior to Fedora 18's end of life.
Thank you for reporting this issue and we are sorry that we may not be
able to fix it before Fedora 18 is end of life. If you would still like
to see this bug fixed and are able to reproduce it against a later version
of Fedora, you are encouraged change the 'version' to a later Fedora
version prior to Fedora 18's end of life.
Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.
Fedora 18 changed to end-of-life (EOL) status on 2014-01-14. Fedora 18 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.
If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
Thank you for reporting this bug and we are sorry it could not be fixed.