Extend tempspace in safe way

[Noremos]

There are a FIXME in the code:

firebird/src/jrd/TempSpace.cpp

Lines 402 to 404 in 8eb7bba

	// NS 2014-07-31: FIXME: missing exception handling.
	// error thrown in block of code below will leave TempSpace in inconsistent state:
	// logical/physical size already increased while allocation has in fact failed.

And seem like it caused a segfault in a prodaction DB due to TempSpace::head is nullptr or TempSpace::tail is nullptr.

I reproduced this using a synteny test

1. Simulate small storage

sudo mount -t tmpfs -o size=1K tmpfs /mnt/ramdisk
sudo mount -t tmpfs -o size=1M tmpfs /mnt/ramdisk 

2. Firebird.conf:

TempDirectories = /mnt/ramdisk
TempCacheLimit = 1M

3. Run the code

First case:

BOOST_CHECK_THROW(space.allocateSpace(tdbb, MORE_THEN_FILE_STORAGE), Firebird::Exception); // <- exception 
pos = space.allocateSpace(tdbb, 100); <- tail is nullptr
space.write(tdbb, pos, "123", 3); // <- segfault

Second case:

const auto MORE_THEN_FILE_STORAGE = 10000024;
BOOST_CHECK_THROW(space.allocateSpace(MORE_THEN_FILE_STORAGE), Firebird::Exception); // <- exception 

pos = space.write(pos, "123", 3); // <- segfault

[hvlad]

At line 375 the only block is deleted, how this recovery handle this case ?

[Noremos]

After an exception occurs, the next call of the write/allocate function will trigger execution of this function. The missing head and tail will be set correctly.

[hvlad]

But the object internal state is inconsistent: it have non-zero size but it have no memory block allocated.
Yes, next write or extend will fix it, but what if next call is read ?

[Noremos]

Indeed, I didn't think about combining read and write operations. I'll improve the fix.

[Noremos]

Now, I delay pointer deliting and restore it in case of an error. But there's a problem with non-dynamic TempSpace. By default, head and tail are NULL. Therefore, if extend fails and the logic ignores the exception, reading can lead to unexpected behavior. But TempSpace::read doesn't dereference a null pointer, so we are safe in any case.

[hvlad]

Looks good, thanks.

The only concern left is - should we convert asserts at the start of read() and write() into exceptions ?
I don't think it adds measurable perf penalty, but adds robustness, imho.

[Noremos]

I think this is a good idea. Especially in the case of new code using TempSpace. I can add this as part of this PR or a new one.

[dyemanov]

I think it fits this PR nicely.

[hvlad]

Me too

[Noremos]

Added

[hvlad]

Use Arg::Int64 as offset_t is 8-byte integer

[aafemt]

Do you think that integer overflow is possible here?

[Noremos]

Do you think that integer overflow is possible here?

Avoiding data truncation is a good practice. At the very least, we won't get an annoying warning from the compiler.