Check list length in GrowPlist #2039

ChrisJefferson · 2017-12-19T20:49:34Z

This ensures GrowPlist checks the size we are growing the list to is a valid list size. This stops us then overflowing in ResizeBag later.

I hope this fixes #1479, but I had trouble reproducing the error originally, so it is hard for me to check.

This also adds two explicit functions to check if a C signed or unsigned integer can be turned into an immediate integer. These functions can be used more widely, but I leave that for another PR (or at least until this one is checked to see if people like them).

codecov · 2017-12-19T22:47:57Z

Codecov Report

Merging #2039 into stable-4.9 will decrease coverage by <.01%.
The diff coverage is 83.33%.

@@              Coverage Diff               @@
##           stable-4.9    #2039      +/-   ##
==============================================
- Coverage       66.01%   66.01%   -0.01%     
==============================================
  Files             898      898              
  Lines          273370   273376       +6     
  Branches        12791    12792       +1     
==============================================
+ Hits           180468   180470       +2     
- Misses          90072    90077       +5     
+ Partials         2830     2829       -1

Impacted Files	Coverage Δ
src/intobj.h	`82.75% <100%> (+1.27%)`	⬆️
src/plist.c	`87.68% <50%> (-0.06%)`	⬇️
src/funcs.c	`76.1% <0%> (-0.41%)`	⬇️
src/hpc/threadapi.c	`34.36% <0%> (-0.29%)`	⬇️
hpcgap/lib/hpc/stdtasks.g	`38.61% <0%> (-0.26%)`	⬇️
src/stats.c	`79.43% <0%> (ø)`	⬆️
src/hpc/traverse.c	`78.76% <0%> (+0.38%)`	⬆️
src/hpc/thread.c	`46.41% <0%> (+0.59%)`	⬆️

fingolfin · 2017-12-19T23:14:34Z

src/gasman.h

@@ -497,7 +497,6 @@ extern  UInt            ResizeBag (
            Bag                 bag,
            UInt                new_size );

-


fingolfin · 2017-12-19T23:16:31Z

src/intobj.h

+*F  INT_CAN_BE_INTOBJ( <i> ) . check if a C integer fits in an immediate
+**  integer.
+*/
+static inline Int INT_CAN_BE_INTOBJ(Int i)


I find this ambiguous -- the name could also be an alternative name for IS_INTOBJ (i.e. "this integer could actually be an intobj, as opposed to being a bag ref"). In the description you use "fits", perhaps also use it in the macro name? So e.g. FITS_INTO_INTOBJ.

fingolfin · 2017-12-19T23:23:08Z

src/intobj.h

+*/
+static inline Int UINT_CAN_BE_INTOBJ(UInt i)
+{
+    return i < 1UL<<NR_SMALL_INT_BITS;


Off-topic remark: Strictly speaking this is not correct, as 1UL indicates an unsigned long, but there are 64bit systems were long is still only 32bits (you need long long for a 64bit type). Specifically, the LLP64 data model, which is what Windows uses on 64bit systems.

The same of course also holds for the 1L above. But of course we already use 1L and 1UL blissfully in lots of places, relying implicitly on all futures 64-systems GAP will run on to use LP64 (which is what Linux, *BSD, OSX use). Portable would be to use the INT64_C and U INT64_C etc. macros, resp. provide our own UINT_C macro (which maps to either UINT64_C or UINT32_C depending on the host bit size.

But again, we already do this wrong in a ton of places, so this PR is not the place to start working on it, I just wanted to mention it because reading that line reminded me of it.

fingolfin · 2017-12-19T23:23:31Z

src/plist.c

@@ -67,6 +67,12 @@ Int             GrowPlist (
    UInt                plen;           /* new physical length             */
    UInt                good;           /* good new physical length        */

+    if(!UINT_CAN_BE_INTOBJ(need))
+    {


Formatting...

ChrisJefferson · 2017-12-20T09:13:41Z

Hopefully all issues now fixed, formatted, random extra lines removed, (hopefully) better casting added.

fingolfin · 2017-12-20T10:12:33Z

src/intobj.h

+*/
+static inline Int INT_FITS_IN_INTOBJ(Int i)
+{
+    return (-((Int)1)<<NR_SMALL_INT_BITS) <= i) && 


Syntax error here, the final closing ) has no matching (.

frankluebeck · 2017-12-20T18:51:53Z

The code change looks fine to me, but it doesn't fix the problems mentioned in #1479 (Crashes from Todd-Coxeter). For example, in the branch of this PR after make clean; ./configure ABI=32; make I get

> bin/gap.sh -r
 ┌───────┐   GAP ChrisJefferson-fix-list-length-2017-12-20 of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default32
 Configuration:  gmp 6.0.0, readline
 Loading the library and packages ...
 Packages:   AClib 1.2, Alnuth 3.0.0, AtlasRep 1.5.0, AutPGrp 1.6, 
             CRISP 1.4.3, Cryst 4.1.12, CrystCat 1.1.6, CTblLib 1.2.2, 
             FactInt 1.5.3, FGA 1.3.1, GAPDoc 1.6.1, IRREDSOL 1.2.4, 
             LAGUNA 3.7.0, Polenta 1.3.6, Polycyclic 2.11, PrimGrp 3.1.2, 
             RadiRoot 2.7, ResClasses 4.4.2, smallgrp 1.2, Sophus 1.23, 
             SpinSym 1.5, TomLib 1.2.5, transgrp 2.0, Utils 0.39
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> l:=[1]; while true do Print("*\c");Append(l,l);od;
[ 1 ]
****************************Error, GrowPlist: List size too large in
  Append( l, l ); at *stdin*:1 called from 
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:1
type 'quit;' to quit to outer loop
brk> 
gap> l:=[1]; while true do Print("*\c");Append(l,l);od;
[ 1 ]
**************************Error, reached the pre-set memory limit
(change it with the -o command line option) in
  Print( "*\c" ); at *stdin*:1 called from 
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:1
you can 'return;'
brk> 
gap> l:=[1]; while true do Print("*\c");Append(l,l);od;
[ 1 ]
***************************Segmentation fault

So, the new code is triggered the first time, but not the second and third time.
Also, the behaviour of the original example in #1479 remains exactly the same as before (i.e., a segfault on the second call).

ChrisJefferson · 2017-12-20T20:07:18Z

I think there is a deeper problem here, we don't need particularly big lists to cause a crash, for example, the following causes a segfault for me.

l:=[1]; while true do l := List([1..10000], x -> l); od;

ChrisJefferson · 2017-12-20T20:16:50Z

Also, when the crash happens, the list is a reasonable length. We just aren't garbage collecting all the lists, memory is being exhausted,.and for some reason a bad thing happens.

fingolfin

We definitely should get this merged

fingolfin · 2017-12-27T11:32:23Z

@ChrisJefferson you write: "This ensures GrowPlist checks the size we are growing the list to is a valid list size. This stops us then overflowing in ResizeBag later". But how exactly do we overflow there?

fingolfin · 2017-12-27T11:34:18Z

I mean, the size of a bag is 32bits. We also store the length of a plist as 32 bits (well, 31 bits, as we currently use Int as type for the length in SET_LEN_PLIST and LEN_PLIST, but of course we could change that.

ChrisJefferson · 2017-12-27T12:17:24Z

So, there are a few things going on here I think, although some might be misinterpreting memmove crashes.

First of all, as the size of a plist must fit in an intermediate integer, there is no reason to ever grow a plist larger than that, and I think having a larger capacity confuses some other pieces of code.

The resizebag problem is that once we get a huge list size, then multiply that by 4 (sizeof(Obj)), then we can overflow the check to see if there is enough free memory for a new.bag.

As a side effect of keeping the size of a plist in a small int, the size of the resulting bag is at most 2^28*4 = 1GB, and all OSes preserve the top 1gb of memory space for kernel (I believe), so we don't overflow that check any more. Note I am away from computer and running on my memory here, so something here might not be quite right.

It is certainly a good idea to not create stupidly large bags, bigger than any plist could require :)

ChrisJefferson · 2017-12-27T12:19:53Z

Note all these comments only apply to 32 bits. There is the same hypothetical problems in 64 bits, but no-one will ever come close to hitting them.

ChrisJefferson · 2017-12-27T12:53:43Z

Also it occured I believed the lengths of plists had to fit in an immediate integer, maybe that isn't true. But I would be willing to bet lots of code assumes it is true.

I really want to understand more precisely what is going on before we merge this.

fingolfin · 2017-12-27T15:21:23Z

So there are several limitations at play here, with different things to do about them:

If ResizeBag can potentially overflow for large target sizes, then we should catch that in there. That said, on a cursory check of the function, I see no overflow problems in ResizeBag. Am I missing something?

As to plists whose does not fit into an immediate integers causing a problem: Well, perhaps, but then, where exactly? Because as @frankluebeck pointed out, some care was already put into supporting that case (at least for strings, but also for other kinds of lists). I'd like to know about specific broken cases. I may just look for some myself, though, not asking you.

Note that we already have two APIs to query the length of a list: LEN_LIST returns an Int, and LENGTH an Obj. With the latter, in principle lists can have arbitrary size (and this may be used; e.g. I could write a custom list "object" which is read-only, whose size is infinity (or at least a huuge integer, like 2^10000, and where accessing entry n just returns n (or whatever else you like). Although I note that the kernel essentially only uses LENGTH in a single place: in FuncIsRectangularTablePlist -- well, and of course to implement the LENGTH kernel attribute...

Anyway, this use of Int (also in LEN_PLIST, as mentioned above) kind of implies a limit of 2^31 (instead of 2^32), at least for the builtin lists (I assume for an external list exceeding that limit, LEN_LIST simply would raise an error).

Then, as @ChrisJefferson pointed out, a plist of length len requires (more than) len*sizeof(Obj) bytes, so on a 32bit system, the size is effectively limited by 2^30. This is the limit I'd tend to enforce then.

fingolfin · 2017-12-27T15:31:08Z

src/plist.c

@@ -67,6 +67,11 @@ Int             GrowPlist (
    UInt                plen;           /* new physical length             */
    UInt                good;           /* good new physical length        */

+    if (!UINT_FITS_IN_INTOBJ(need)) {
+        ErrorMayQuit("GrowPlist: List size too large", 0, 0);
+    }


This check is actually not enough: A few lines below, we also "round" up the current capacity of the list, i.e.

good = 5 * CAPACITY_PLIST(list) / 4 + 4;

So if the list was close to the limit before, then good will exceed the limit -- and then, even if need is below the limit, we end up using good (which is larger than need) as the new list length.

I think the correct solution then is to first check here; then compute good, and change

if ( need < good ) { plen = good; } else { plen = need; }

to something like

if ( need < good && UINT_FITS_IN_INTOBJ(good) ) plen = good; else plen = need;

ChrisJefferson · 2017-12-27T17:01:28Z

All of the intobj_int in listoper.c look like places where we assume plists (and in some cases lists I think :( ) indices fit in immediate integers.

The overflow I thought I found was caused by a pointer overflow, if current upper limit + new bag size overflowed, but now I can't find it.

Note i never thought the size of lists was required to fit in immediate integer, only plists.

fingolfin · 2017-12-27T19:08:29Z

OK, the point with listoper.c (and also listfunc.c) is kind of convincing (although of course we could also extend them to deal with larger sizes, but I don't believe it's worth the effort: on 64bit, it's completely irrelevant, and on 32 bit, nobody ever requested it. So, let's go with the direction of this PR -- though as I wrote in a source comment, this still needs a check to also bound the value of good resp. plen.

fingolfin · 2017-12-27T19:29:16Z

@ChrisJefferson I pushed a possible fixup to this PR. If you agree, please rebase it (and squash the fixup) -- I can't do that with your PR.

ChrisJefferson · 2017-12-27T19:37:23Z

Unfortunately I am literally on my phone until Jan 3rd or so. If you want to make a new PR, feel free!

fingolfin · 2018-01-04T10:09:38Z

Closing this in favor of PR #2064

fingolfin reviewed Dec 19, 2017

View reviewed changes

ChrisJefferson force-pushed the fix-list-length branch from bfdbbee to 5cfbfb9 Compare December 20, 2017 09:13

fingolfin reviewed Dec 20, 2017

View reviewed changes

ChrisJefferson added 2 commits December 20, 2017 12:05

Add functions to check if a C int fits in an immediate GAP int

8b32bdc

Add bounds checking to GrowPlist

394727c

ChrisJefferson force-pushed the fix-list-length branch from 5cfbfb9 to 394727c Compare December 20, 2017 12:05

fingolfin mentioned this pull request Dec 20, 2017

Crashes from Todd Coxeter #1479

Closed

fingolfin previously approved these changes Dec 27, 2017

View reviewed changes

fingolfin added kind: bug Issues describing general bugs, and PRs fixing them topic: kernel labels Dec 27, 2017

fingolfin added this to the GAP 4.9.0 milestone Dec 27, 2017

fingolfin requested changes Dec 27, 2017

View reviewed changes

fixup! Add bounds checking to GrowPlist

6b07c8d

fingolfin mentioned this pull request Jan 4, 2018

Restrict capacity of plists to 2^28 resp. 2^60 (revision of #2039) #2064

Merged

fingolfin closed this Jan 4, 2018

ChrisJefferson deleted the fix-list-length branch February 19, 2018 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check list length in GrowPlist #2039

Check list length in GrowPlist #2039

ChrisJefferson commented Dec 19, 2017

codecov bot commented Dec 19, 2017 •

edited

Loading

fingolfin Dec 19, 2017

fingolfin Dec 19, 2017

fingolfin Dec 19, 2017

fingolfin Dec 19, 2017

ChrisJefferson commented Dec 20, 2017

fingolfin Dec 20, 2017

frankluebeck commented Dec 20, 2017

ChrisJefferson commented Dec 20, 2017

ChrisJefferson commented Dec 20, 2017

fingolfin left a comment

fingolfin commented Dec 27, 2017

fingolfin commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

fingolfin commented Dec 27, 2017

fingolfin Dec 27, 2017 •

edited

Loading

ChrisJefferson commented Dec 27, 2017 •

edited

Loading

fingolfin commented Dec 27, 2017

fingolfin commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

fingolfin commented Jan 4, 2018

		@@ -497,7 +497,6 @@ extern UInt ResizeBag (
		Bag bag,
		UInt new_size );

Check list length in GrowPlist #2039

Check list length in GrowPlist #2039

Conversation

ChrisJefferson commented Dec 19, 2017

codecov bot commented Dec 19, 2017 • edited Loading

Codecov Report

fingolfin Dec 19, 2017

Choose a reason for hiding this comment

fingolfin Dec 19, 2017

Choose a reason for hiding this comment

fingolfin Dec 19, 2017

Choose a reason for hiding this comment

fingolfin Dec 19, 2017

Choose a reason for hiding this comment

ChrisJefferson commented Dec 20, 2017

fingolfin Dec 20, 2017

Choose a reason for hiding this comment

frankluebeck commented Dec 20, 2017

ChrisJefferson commented Dec 20, 2017

ChrisJefferson commented Dec 20, 2017

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin commented Dec 27, 2017

fingolfin commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

fingolfin commented Dec 27, 2017

fingolfin Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

ChrisJefferson commented Dec 27, 2017 • edited Loading

fingolfin commented Dec 27, 2017

fingolfin commented Dec 27, 2017

ChrisJefferson commented Dec 27, 2017

fingolfin commented Jan 4, 2018

codecov bot commented Dec 19, 2017 •

edited

Loading

fingolfin Dec 27, 2017 •

edited

Loading

ChrisJefferson commented Dec 27, 2017 •

edited

Loading