#699 Bug on the "New[]" operator when attempting to allocate a negative number of elements

[fxm (freebasic.net)]

result = New datatype[ count ]

• if count<0, runtime error:
  "terminate called after throwing an instance of 'std::bad_alloc'
  what(): std::bad_alloc"
  "This application has requested the Runtime to terminate it in an unusual way.
  Please contact the application's support team for more information."
• If a negative constant value is assigned: "New datatype[ -1 ]" for example and specifier "{Any}" not used, the compiler even hangs and fbc.exec must be manually killed!
• if count>=0, a non null pointer is returned (even for count=0)

I think that when count<0 (constant value assigned or variable), the compiler must not hang and a null pointer should be always returned.

---

Comparison with Allocate, Callocate, Reallocate

result = [C]Allocate( count )
- if count<0, a null pointer is returned
- if count>=0, a non nul pointer is returned (even for count=0)

result = Reallocate( pointer, count )
- if count<0, a null pointer is returned
- if count=0 and pointer=0, a non null pointer is returned (as similar to [C]Allocate behavior)
- if count=0 and pointer>0, a null pointer is returned (syntax equivalent to Deallocate)
- if count>0, a non null pointer is returned

---

Referring to forum at post:
http://www.freebasic.net/forum/viewtopic.php?p=190454#p190454

[fxm (freebasic.net)]

Update of the "New[count]" behavior analysis, because for count=0, it depends of the data-type:

result = New datatype[ count ]

• if count<0, runtime error:
  "terminate called after throwing an instance of 'std::bad_alloc'
  what(): std::bad_alloc"
  "This application has requested the Runtime to terminate it in an unusual way.
  Please contact the application's support team for more information."
• If a negative constant value is assigned: "New datatype[ -1 ]" for example and specifier "{Any}" not used, the compiler even hangs and fbc.exec must be manually killed!
• if count=0, a non null pointer is returned (for pre-defined data-type for example) or the program is aborted without error message (for UDT with constructor for example)!
• if count>0, a non null pointer is returned

I think that when count<=0 (constant value assigned or variable), the compiler must not hang and a null pointer should be always returned.

[Matthew]

For constant negative values, fbc is getting stuck in emit_x86.bas, emitting code like the following:

push 4294967292
call __Znaj
add esp, 4
mov dword ptr [ebp-8], eax
mov dword ptr [ebp-8], 0
mov dword ptr [ebp-4], 0
mov dword ptr [ebp], 0
mov dword ptr [ebp+4], 0
mov dword ptr [ebp+8], 0
...

From this I'm guessing:
1. There will be 4294967295 lines like this, wrapping around nearly four times in 32-bit mode.
2. There is no apparent check to make sure the allocation succeeded.
3. There is no overflow check (2^32-1 4-byte Integers obviously can't be addressed in a 32-bit memory space), and the -1*sizeof(integer) size calculation has silently overflowed to 4294967292.

[dkl]

astNewMEM() and the emit_x86.bas:hMemClear*() functions have checks to emit a rep stosd to clear huge amounts of memory, instead of emitting tons of mov's to initialize every dword to zero manually.

The negative size value (signed Integers used to hold it) breaks those "is it too much to initialize manually?" checks in astNewMEM() and hMemClear(). Then hMemClearBlkIMM() gets called which does a cunsg() when emitting the mov's.

I think that part definitely needs fixing, because the compiler shouldn't try to emit gigabytes of ASM code either way (seemingly getting stuck). Besides that though, it should be noted that New and Allocate functions have unsigned parameters, so negative values that are passed in will wrap around and be seen as very huge values. For -1 becoming 4 GiB, the system will probably refuse to allocate that much and return NULL.

The report about std::bad_alloc must be coming from GCC's libraries or the system; as another example, on Linux I get a simple "Aborted" message. I think that's because FB calls the exception-throwing versions of the new/delete operators from GCC's libsupc++. But since we do no exception handling in FB I'm guessing some of the CRT/GCC code ends up calling abort().

I'm thinking we could either ensure to call the non-exception-throwing versions of new/delete operators (which I believe exists, with different name mangling of course), or provide our own new/delete operators in the rtlib, or simply call malloc/free as I don't think the new/delete operators from libsupc++ do anything else anyways. Interestingly, that would remove FB's dependency on libsupc++, I think.

[dkl]

The "compiler hang" should now be fixed by [1c24eb].

Not sure what to do about the exception throwing, except not using C++'s new/delete operators in the first place. I'd actually like that (as mentioned, dropping the dependency on libsupc++, that would be great), but I'll have to do some research first to figure out the possible consequences...

[dkl]

This should be solved now: [8a9501], [8bfe9a]

[fxm (freebasic.net)]

One case of program aborting, the one for 'result = New UDT[ 0 ]' and UDT having for example a constructor, is not solved.

Example:

Type UDT
  Dim As Byte b
  Declare Constructor ()
End Type

Constructor UDT ()
  Print "Intance construction " & @This
End Constructor

Dim As UDT Ptr p = New UDT[0]

Intance construction 10168928
Intance construction 10168929
Intance construction 10168930
Intance construction 10168931
Intance construction 10168932
.....
.....
Aborting due to runtime error 12 ("segmentation violation" signal) in C:\Users\fxmam\Documents\Mes Outils Personnels\FBIde0.4.6r4_fbc1.06.0\FBIDETEMP.bas::()

Must I create a new bug report?

[Jeff Marshall]

fxm, OK, will just re-open this ticket and refer to the forum post.

[Jeff Marshall]

Started pull request https://github.com/freebasic/fbc/pull/100

The pull request fixes some, but not all, of the remaining issues with this bug.

• new[N] expects N to be unsigned, so internally, use the UINT type for this expression
• for new[const N], because const N is expected to be unsigned, added a warning if constant is signed and out of range (negative). This is not fully correct; see below
• new[expr] was causing infinite loop when expr=0 because of the previous For/Next constructor iterator always starts with at least one iteration. expr may be an expression that is only known at run-time. Solved this by adding a While(CNT) iterator for ctor/dtor loop.
• if new[expr] fails (NULL ptr returned), added IF/ENDIF to prevent the constructor iterator loop from executing on a NULL pointer. NULL pointer is returned from the new[] expression instead.

The last issue, not solved is the limits on NEW T[N]:

• N is expected to be unsigned integer
• practically though, (N * sizeof(T) + sizeof(integer)) should not be allowed to overflow unsigned integer otherwise we get a bad memory allocation
• misleading is the specific test case of new T[-1] where sizeof(T)=sizeof(integer), -1 is converted to UINT, and the allocation request ends up being zero (0) bytes. That's incidental only, because, for example in 32-bit the allocation is (&hffffffff * 4 + 4) which is same case of overflow as mentioned above.

[Jeff Marshall]

Merged pull request 100 in to fbc/master.

I added a note about overflowing the allocation at https://www.freebasic.net/wiki/wikka.php?wakka=KeyPgOpNew

I think the reported bugs are solved, so am closing this report.

[Jeff Marshall]

fixed in [f081eea86d6ae21aa2abf7cdeaa54cf56cb8b6e0]