Skip to content
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.

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

Fix SEGV in blosc2_schunk_free() #637

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

EmilDohne
Copy link
Contributor

Apologies for only getting around to it now, this should address the recent regression in 2.15.0 :)

Context:

#635 I noticed while upgrading to c-blosc2 2.15.0 that calling blosc2_schunk_free() without blosc2_init() being called first caused a SEGV as it was trying to access uninitialized memory

Changes

  • Added check to blosc2_get_io_cb() that returns NULL if the library was not initialized yet so we don't try to access a nullptr function in blosc2_schunk_free()
  • Added test cases for "manual" compression and decompression using blosc2_compress_ctx and blosc2_decompress_ctx. This does not in parallel but does trigger a test failure before my changes.

Considerations

@FrancescAlted I noticed that blosc2_destroy() does not actually clean up the IO callbacks (note the order of tests, if it was reversed the tests would pass even without the fix). Is this intended?

@EmilDohne
Copy link
Contributor Author

I think the error is because clang didnt like the missing return type which I missed. I've also gone ahead and address some of the unused variable warnings. Hopefully it passes CI now!

@FrancescAlted
Copy link
Member

I think the error is because clang didnt like the missing return type which I missed. I've also gone ahead and address some of the unused variable warnings. Hopefully it passes CI now!

Yes, your fix did it, at least on my mac laptop. I am currently traveling, but I'll come back to you later.

Regarding blosc2_destroy() not cleaning up I/O calls is probably an oversight. Can you fix this too?

@EmilDohne
Copy link
Contributor Author

Regarding blosc2_destroy() not cleaning up I/O calls is probably an oversight. Can you fix this too?

I'll have a deeper look into this and see if it needs to be cleaned up. Enjoy your travels!

@EmilDohne
Copy link
Contributor Author

Looks like this was also the reason for the segfaults in the aarch tests :)

@EmilDohne
Copy link
Contributor Author

EmilDohne commented Oct 23, 2024

Ok so I had a look into the cleanup stuff and at least from a memory standpoint there is no concerns. One could, if wanted, set these back to be default initialized but I wouldn't personally do that

// In blosc2_init()

BLOSC2_IO_CB_DEFAULTS.id = BLOSC2_IO_FILESYSTEM;
BLOSC2_IO_CB_DEFAULTS.name = "filesystem";
BLOSC2_IO_CB_DEFAULTS.is_allocation_necessary = true;
BLOSC2_IO_CB_DEFAULTS.open = (blosc2_open_cb) blosc2_stdio_open;
BLOSC2_IO_CB_DEFAULTS.close = (blosc2_close_cb) blosc2_stdio_close;
BLOSC2_IO_CB_DEFAULTS.size = (blosc2_size_cb) blosc2_stdio_size;
BLOSC2_IO_CB_DEFAULTS.write = (blosc2_write_cb) blosc2_stdio_write;
BLOSC2_IO_CB_DEFAULTS.read = (blosc2_read_cb) blosc2_stdio_read;
BLOSC2_IO_CB_DEFAULTS.truncate = (blosc2_truncate_cb) blosc2_stdio_truncate;
BLOSC2_IO_CB_DEFAULTS.destroy = (blosc2_destroy_cb) blosc2_stdio_destroy;

BLOSC2_IO_CB_MMAP.id = BLOSC2_IO_FILESYSTEM_MMAP;
BLOSC2_IO_CB_MMAP.name = "filesystem_mmap";
BLOSC2_IO_CB_MMAP.is_allocation_necessary = false;
BLOSC2_IO_CB_MMAP.open = (blosc2_open_cb) blosc2_stdio_mmap_open;
BLOSC2_IO_CB_MMAP.close = (blosc2_close_cb) blosc2_stdio_mmap_close;
BLOSC2_IO_CB_MMAP.read = (blosc2_read_cb) blosc2_stdio_mmap_read;
BLOSC2_IO_CB_MMAP.size = (blosc2_size_cb) blosc2_stdio_mmap_size;
BLOSC2_IO_CB_MMAP.write = (blosc2_write_cb) blosc2_stdio_mmap_write;
BLOSC2_IO_CB_MMAP.truncate = (blosc2_truncate_cb) blosc2_stdio_mmap_truncate;
BLOSC2_IO_CB_MMAP.destroy = (blosc2_destroy_cb) blosc2_stdio_mmap_destroy;

From my side there is nothing else to add so feel free to merge :)

@FrancescAlted
Copy link
Member

LGTM. Thanks @EmilDohne !

@FrancescAlted FrancescAlted merged commit 99e8c42 into Blosc:main Oct 23, 2024
17 checks passed
@EmilDohne EmilDohne deleted the fix-segv-in-blosc2-schunk-free branch October 23, 2024 18:59
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