Skip to content

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Sep 9, 2024

Just something I noticed while working on something related to the buffer manager.

BufferHandle takes a BlockHandle and a FileBuffer pointer, but it's a raw pointer so unless the lifetime is guaranteed by an outside scope, providing anything other than the handle->buffer.get() can cause a heap use after free issue.

Tracing back the lineage, this originates from an age before we had BlockHandle: Mytherin@7251a55#diff-bf94710a9036c38ee30044ac99f5752dc2bddfda44cd3f82992d992501f3d956

@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 9, 2024 14:30
@Tishj Tishj marked this pull request as ready for review September 9, 2024 14:30
@Tishj
Copy link
Contributor Author

Tishj commented Sep 9, 2024

I also have a couple follow up changes to this, a bigger one is changing the prototype of BufferManager::Allocate

from:

	virtual BufferHandle Allocate(MemoryTag tag, idx_t block_size, bool can_destroy = true,
	                              shared_ptr<BlockHandle> *block = nullptr) = 0;

to:

	virtual BufferHandle Allocate(MemoryTag tag, idx_t block_size, bool can_destroy = true) = 0;

The block parameter is not used, it's only used as an "out" parameter.
Instead of this extremely confusing looking prototype (to me atleast), we can just use BufferHandle::GetBlockHandle on the result if we want to get the BlockHandle

@Tishj Tishj requested a review from Mytherin September 10, 2024 07:49
@Mytherin Mytherin merged commit 6a197b2 into duckdb:main Sep 10, 2024
41 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Sep 25, 2024
Merge pull request duckdb/duckdb#13823 from Tishj/buffer_handle_cleanup
Merge pull request duckdb/duckdb#13821 from Tishj/python_3_13_fix
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Sep 25, 2024
Merge pull request duckdb/duckdb#13823 from Tishj/buffer_handle_cleanup
Merge pull request duckdb/duckdb#13821 from Tishj/python_3_13_fix

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants