This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 850911 - g++: operator new[] overflow fix
g++: operator new[] overflow fix
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: gcc (Show other bugs)
18
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Jakub Jelinek
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 853918
  Show dependency treegraph
 
Reported: 2012-08-22 13:13 EDT by Florian Weimer
Modified: 2014-02-05 07:05 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-02-05 07:05:49 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
operator-new.patch (20.83 KB, patch)
2012-08-22 13:13 EDT, Florian Weimer
no flags Details | Diff
operator-new-with-warning.patch (17.16 KB, patch)
2012-08-31 08:29 EDT, Florian Weimer
no flags Details | Diff
Diff to previous patch (9.62 KB, patch)
2012-08-31 08:31 EDT, Florian Weimer
no flags Details | Diff
gcc47-pr19351.patch (15.57 KB, patch)
2012-09-27 10:20 EDT, Jakub Jelinek
no flags Details | Diff
new39a.C (1.09 KB, text/plain)
2012-09-27 11:14 EDT, Jakub Jelinek
no flags Details
new39b.C (1008 bytes, text/plain)
2012-09-27 11:15 EDT, Jakub Jelinek
no flags Details

  None (edit)
Description Florian Weimer 2012-08-22 13:13:42 EDT
Created attachment 606350 [details]
operator-new.patch

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.
Comment 1 Florian Weimer 2012-08-31 08:29:32 EDT
Created attachment 608498 [details]
operator-new-with-warning.patch

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.
Comment 2 Florian Weimer 2012-08-31 08:31:05 EDT
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).
Comment 3 Jakub Jelinek 2012-09-27 10:20:55 EDT
Created attachment 618101 [details]
gcc47-pr19351.patch

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.
Comment 4 Jeff Law 2012-09-27 11:07:32 EDT
Jakub,

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.
Comment 5 Jakub Jelinek 2012-09-27 11:13:35 EDT
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.
Comment 6 Jakub Jelinek 2012-09-27 11:14:34 EDT
Created attachment 618142 [details]
new39a.C

One of the VLA testcases I used for testing this (adapted init/new39.C for VLAs).
Comment 7 Jakub Jelinek 2012-09-27 11:15:37 EDT
Created attachment 618143 [details]
new39b.C

And another testcase (new39a.C is to catch the overflow exceptions, this one to test some smaller sizes that must be accepted).
Comment 8 Jeff Law 2012-09-27 11:15:58 EDT
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.

jeff
Comment 9 Florian Weimer 2012-09-27 15:39:09 EDT
(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.
Comment 10 Jakub Jelinek 2012-09-27 15:44:31 EDT
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.
Comment 11 Florian Weimer 2012-09-27 16:11:04 EDT
(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
> that.

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.
Comment 12 Jakub Jelinek 2012-10-01 09:29:31 EDT
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.
Comment 13 Fedora End Of Life 2013-12-21 03:48:35 EST
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.
Comment 14 Fedora End Of Life 2014-02-05 07:05:53 EST
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
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.

Note You need to log in before you can comment on or make changes to this bug.