SEGV in blosc2_schunk_free()

[EmilDohne]

Describe the bug
I noticed while upgrading from v2.11.2. -> 2.15.1. that I now receive a SEGV on calling blosc2_schunk_free().
Reverting back down to 2.14.0 fixes this problem. I was able to narrow it down to the 2.15.0 release.

The actual code calling blosc2_schunk_free() can be seen here

As far as I'm aware I'm not doing anything that is unintended here but feel free to correct me

Logs
Below you can find some gh action runs across multiple OSs where these errors can be seen

Valgrind 2.15.1
Asan 2.15.1

Valgrind 2.14.0
Asan 2.14.0 <-- There is some potentially unrelated issues in the GCC ubuntu build here

System information:

• OS: Windows | Ubuntu | MacOS
• Compiler GCC | Clang | MSVC
• Version 2.15.0

[EmilDohne]

I was able to narrow this issue down to the following lines in blosc2_schunk_free():

  if (schunk->storage != NULL) {
    blosc2_io_cb *io_cb = blosc2_get_io_cb(schunk->storage->io->id);
    if (io_cb != NULL) {
      int rc = io_cb->destroy(schunk->storage->io->params); // <----- io_cb->destroy is empty
      if (rc < 0) {
        BLOSC_TRACE_ERROR("Could not free the I/O resources.");
      }
    }

This seems to occur because in this function:

blosc2_io_cb *blosc2_get_io_cb(uint8_t id) {
  for (uint64_t i = 0; i < g_nio; ++i) {
    if (g_ios[i].id == id) {
      return &g_ios[i];
    }
  }
  if (id == BLOSC2_IO_FILESYSTEM) {
    if (_blosc2_register_io_cb(&BLOSC2_IO_CB_DEFAULTS) < 0) {
      BLOSC_TRACE_ERROR("Error registering the default IO API");
      return NULL;
    }
    return blosc2_get_io_cb(id);
  }
  else if (id == BLOSC2_IO_FILESYSTEM_MMAP) {
    if (_blosc2_register_io_cb(&BLOSC2_IO_CB_MMAP) < 0) {
      BLOSC_TRACE_ERROR("Error registering the mmap IO API");
      return NULL;
    }
    return blosc2_get_io_cb(id);
  }
  return NULL;
}

When passed an id of filesystem (0, default value) we call _blosc2_register_io_cb for this filesystem which then returns a default initialized io_cb which contains nullptrs such as io_cb->destroy.

One solution would be to check for this nullptr

if (io_cb != NULL && io_cb->destroy != NULL) {

}

Another, breaking, solution would be to reconsider whether having filesystem as default is a good idea and if instead there should be an in-memory enum such as BLOSC2_IO_NONE which then in blosc2_get_io_cb would just return a nullptr

[FrancescAlted]

Good investigation work @EmilDohne . We could break ABI again by bumping SOVERSION, although I'd prefer a more conservative solution. @JanSellner what do you think?

[EmilDohne]

I'd be open to other solutions as well, I have to admit I'm not very familiar with the low level codebase of c-blosc2 and only went as deep as my debugger took me :)

[JanSellner]

Both IO types (default and mmap) have a default function for destroy(), so io_cb->destroy should actually never be NULL for the default IOs.

@EmilDohne Do you have an idea why io_cb->destroy is NULL in your case? Do you use a custom IO?

A simple extension of the check towards && io_cb->destroy != NULL costs basically nothing so if that fixes the problem, I would go for it. This would cover the use case if someone has a custom IO but did not provide a destroy() callback.

[EmilDohne]

@JanSellner thanks for your insights. I think I was able to narrow it down to misuse on my end that never manifested itself until now.

It appears I was missing a call to blosc2_init() which it appears is always necessary now if you wish to free blosc2 with a memory backed buffer as otherwise the call to blosc2_schunk_free() will always try to grab an uninitialized io_cb which will have a nullptr in it.

As I would prefer not having to call blosc2_init() and blosc2_free() as my API doesn't necessarily have a defined entry and exit point perhaps it would be better to check if g_initlib is false and just return a nullptr in that case?

It might also be an idea to add tests using blosc2 in a multithreaded context without calls to blosc2_init() and blosc2_free() but instead with blosc2_compress_ctx() and blosc2_decompress_ctx()

[FrancescAlted]

Happy to see that you have found the root of issue. I like the idea of checking for g_initlib and return a nullptr. Also +1 for adding tests for blosc2_compress_ctx() and blosc2_decompress_ctx() without calls to blosc2_init() and blosc2_free(). Would you like to contribute a PR for this?

[EmilDohne]

I'd be happy to take a look at implementing this! I'll likely only get around to it tomorrow :)

[EmilDohne]

Fixed by #637