Avoid null pointer dereference in dt_mipmap_cache_get_with_caller

[kofa73]

Guard against null pointer dereference when _thumbs_prefetch calls dt_mipmap_cache_get_with_caller with buf = NULL.
Fixes #18249.

[kofa73]

The pointer can be dereferenced (subject to certain conditions) elsewhere, too:

  if(flags == DT_MIPMAP_TESTLOCK)
  {
    // simple case: only get and lock if it's there.
    dt_cache_entry_t *entry = dt_cache_testget(&_get_cache(cache, mip)->cache, key, mode);
    buf->cache_entry = entry;

  else if(flags == DT_MIPMAP_BLOCKING)
  {
    // simple case: blocking get
    dt_cache_entry_t *entry =  dt_cache_get_with_caller(&_get_cache(cache, mip)->cache, key, mode, file, line);

    ASAN_UNPOISON_MEMORY_REGION(entry->data, dt_mipmap_buffer_dsc_size);

    struct dt_mipmap_buffer_dsc *dsc = (struct dt_mipmap_buffer_dsc *)entry->data;
    buf->cache_entry = entry;

  else if(flags == DT_MIPMAP_BEST_EFFORT)
  {
    __sync_fetch_and_add(&(_get_cache(cache, mip)->stats_requests), 1);
    // best-effort, might also return NULL.
    // never decrease mip level for float buffer or full image:
    dt_mipmap_size_t min_mip = (mip >= DT_MIPMAP_F) ? mip : DT_MIPMAP_0;
    for(int k = mip; k >= min_mip && k >= 0; k--)
    {
      // already loaded?
      dt_mipmap_cache_get(cache, buf, imgid, k, DT_MIPMAP_TESTLOCK, 'r');
      if(buf->buf && buf->width > 0 && buf->height > 0)

Also, should I update

      if(g_file_test(filename, G_FILE_TEST_EXISTS))
        dt_mipmap_cache_get(cache, 0, imgid, DT_MIPMAP_0, DT_MIPMAP_PREFETCH_DISK, 0);

to use NULL instead of 0 (mipmap_cache.c#dt_mipmap_cache_get_with_caller, line 1137?

[jenshannoschwalm]

There are quite a number of dereferencing issues and even race conditions with mipmaps and image structs pending. Many get fixed via #18061 , there is a pending PR here locally doing more. I will ping you after merging #18061 and opening the next PR for review.

[kofa73]

@jenshannoschwalm : I think it may make sense to merge this tiny change. In culling.c, dt_mipmap_cache_get is called with an explicit null, and the log statement does not check it. One needs both -d verbose and -d cache to hit this, but it's 100% reproducible if I have images in my library.

darktable/src/dtgtk/culling.c

Line 1170 in 8fb4ae6

dt_mipmap_cache_get(darktable.mipmap_cache, NULL, id, mip, DT_MIPMAP_PREFETCH, 'r');

darktable/src/dtgtk/culling.c

Line 1207 in 8fb4ae6

dt_mipmap_cache_get(darktable.mipmap_cache, NULL, id, mip, DT_MIPMAP_PREFETCH, 'r');

[jenshannoschwalm]

Full agreement from my side.

[jenshannoschwalm]

@kofa73 why not squash both to make it more clear?

@TurboGit i would take it :-)

[kofa73]

Done, thanks

[TurboGit]

Fully safe :) Thanks.