Skip to content

Extend tempspace in safe way #9030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Noremos wants to merge 5 commits into
base: master
Choose a base branch
from
+32 −7
Open

Extend tempspace in safe way #9030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
533f040
Extend tempspace in safe way
May 18, 2026
54ba0ec
Restore `head` and `tail` in `TempSpace::extend` in case of exception
May 19, 2026
a045647
Better name for original head holder in TempSpace::extend
May 19, 2026
182dfb0
Add exception instead of assert for TempSpace::read and TempSpace::write
May 19, 2026
ca7e826
Use Arg::Int64 in TempSpace status_vector
May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/include/firebird/impl/msg/jrd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,3 +1015,4 @@ FB_IMPL_MSG(JRD, 1012, bad_constant_type, -901, "2F", "000", "@1 is not supporte
FB_IMPL_MSG(JRD, 1013, not_defined_constant, -901, "42", "000", "The constant @1 is not defined")
FB_IMPL_MSG(JRD, 1014, const_name, -901, "42", "000", "CONSTANT @1")
FB_IMPL_MSG(JRD, 1015, private_table, -901, "42", "000", "Table @1 is private to package @2")
FB_IMPL_MSG(JRD, 1016, temp_space_invalid_pos, -901, "HY", "000", "Invalid position to read/write in a temporary file (positon: @1, size: @2)")
1 change: 1 addition & 0 deletions src/include/gen/Firebird.pas
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5972,6 +5972,7 @@ IPerformanceStatsImpl = class(IPerformanceStats)
isc_not_defined_constant = 335545333;
isc_const_name = 335545334;
isc_private_table = 335545335;
isc_temp_space_invalid_pos = 335545336;
isc_gfix_db_name = 335740929;
isc_gfix_invalid_sw = 335740930;
isc_gfix_incmp_sw = 335740932;
Expand Down
37 changes: 30 additions & 7 deletions src/jrd/TempSpace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,11 @@ TempSpace::~TempSpace()

FB_SIZE_T TempSpace::read(offset_t offset, void* buffer, FB_SIZE_T length)
{
fb_assert(offset + length <= logicalSize);
if (offset + length > logicalSize)
{
status_exception::raise(Arg::Gds(isc_temp_space_invalid_pos) <<
Arg::Int64(offset + length) << Arg::Int64(logicalSize));
}

if (length)
{
Expand Down Expand Up @@ -290,7 +294,11 @@ FB_SIZE_T TempSpace::read(offset_t offset, void* buffer, FB_SIZE_T length)

FB_SIZE_T TempSpace::write(offset_t offset, const void* buffer, FB_SIZE_T length)
{
fb_assert(offset <= logicalSize);
if (offset > logicalSize)
{
status_exception::raise(Arg::Gds(isc_temp_space_invalid_pos) <<
Arg::Int64(offset) << Arg::Int64(logicalSize));
}

if (offset + length > logicalSize)
{
Expand Down Expand Up @@ -328,9 +336,17 @@ FB_SIZE_T TempSpace::write(offset_t offset, const void* buffer, FB_SIZE_T length

void TempSpace::extend(FB_SIZE_T size)
{
const auto originalLogicalSize = logicalSize;
const auto originalPhysicalSize = physicalSize;

AutoPtr<Block> originalHead; // Delay deletion and restore in case of error
Block* originalTail = tail;

logicalSize += size;
if (logicalSize <= physicalSize)
return;

if (logicalSize > physicalSize)
try
{
const FB_SIZE_T initialSize = initialBuffer.getCount();

Expand Down Expand Up @@ -367,7 +383,7 @@ void TempSpace::extend(FB_SIZE_T size)
if (initialSize)
{
fb_assert(head == tail);
delete head;
originalHead = head;
head = tail = NULL;
size = static_cast<FB_SIZE_T>(FB_ALIGN(logicalSize, minBlockSize));
physicalSize = size;
Expand Down Expand Up @@ -399,12 +415,10 @@ void TempSpace::extend(FB_SIZE_T size)
}
}

// 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.
if (!block)
{
// allocate block in the temp file
// Possible error thrown when not enough physical memory
TempFile* const file = setupFile(size);
fb_assert(file);
if (tail && tail->sameFile(file))
Expand All @@ -430,6 +444,15 @@ void TempSpace::extend(FB_SIZE_T size)
}
tail = block;
}
catch (...)
{
// Restore original state
logicalSize = originalLogicalSize;
physicalSize = originalPhysicalSize;
head = originalHead.release();
tail = originalTail;
throw;
Copy link
Copy Markdown
Member

@hvlad hvlad May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Noremos Noremos May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@hvlad hvlad May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Contributor Author

@Noremos Noremos May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Noremos Noremos May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@hvlad hvlad May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@Noremos Noremos May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@dyemanov dyemanov May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it fits this PR nicely.

Copy link
Copy Markdown
Member

@hvlad hvlad May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too

Copy link
Copy Markdown
Contributor Author

@Noremos Noremos May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

}
}

//
Expand Down
Loading