blender+darktable: 3 new defects, MOADs 0002-0005 scan complete

blender-0004: MOAD-0001 CWE-407 anim_channels_edit.cc
  rearrange_animchannel_islands() calls BLI_findptr(anim_data_visible, channel, ...)
  inside the channel-grouping loop — O(C*V) where C=channels, V=visible channels.
  In a complex rig: C=V=1000, 1,000,000 pointer comparisons per reorder.
  Fix: build blender::Set<void*> from anim_data_visible before loop, O(1) lookup.
  Measured: 250x op-count ratio at C=V=1000.

darktable-0004: MOAD-0004 CWE-312 pwstorage backends
  backend_kwallet.c and backend_apple_keychain.c log credential key/value pairs
  verbatim via dt_print(DT_DEBUG_PWSTORAGE, "storing (%s, %s)", key, value).
  `value` for Piwigo export is JSON including plaintext password.
  Triggered by `darktable -d pwstorage` or `-d all` (common debugging mode).
  Fix: replace value argument with "[REDACTED]" in all four dt_print calls.

darktable-0005: MOAD-0001 CWE-407 modulegroups test_visible O(M*G*P)
  _lib_modulegroups_update_iop_visibility() iterates M=80 IOP modules, calling
  _lib_modulegroups_test_visible() which iterates G=8 groups doing g_list_find_custom
  (linear scan of P=10 module names per group) — O(M*G*P) per UI refresh.
  Called on every search keystroke, module toggle, and group switch.
  Fix: precompute GHashTable of all visible module names; test_visible = O(1).
  Measured: 37.8x op-count ratio at M=80, G=8, P=10.

MOADs 0002/0003/0005 CLEAN for blender; MOADs 0002/0003/0005 CLEAN for darktable.
All 4 blender + 5 darktable unit tests PASS.
This commit is contained in:
russell@unturf.com 2026-03-31 20:49:15 -04:00
parent 595e96ffe1
commit 7d135aa6a1
12 changed files with 467 additions and 7 deletions

View file

@ -0,0 +1,8 @@
MOAD-0002 (Intertangle): CLEAN
darktable uses a global `darktable` struct that holds all subsystem pointers
(darktable.db, darktable.undo, darktable.develop, darktable.pwstorage, etc.).
This is intentional architecture: darktable is a single-process application
with tight subsystem coupling by design. The subsystems do not independently
evolve and share a single GTK main thread. No accidental Intertangle coupling
between independently-deployable subsystems found.

View file

@ -0,0 +1,8 @@
MOAD-0003 (Leaked Context): CLEAN
darktable uses `static __thread int32_t threadid` in control/jobs.c to track
numeric worker thread pool indices. This is not request-scoped identity: it is
a pool slot number used for selecting per-thread OpenCL queues and cache
entries. It carries no user-visible identity (image ID, job ID, session). The
correct fix pattern (ScopedValue / ContextVar) does not apply here. No Leaked
Context defect found.

View file

@ -0,0 +1,8 @@
MOAD-0005 (Thundering Herd / CWE-362): CLEAN
darktable's image cache (src/common/cache.c) uses a single dt_pthread_mutex_t
covering all get/insert/release operations. The dt_cache_get function acquires
the lock before hash lookup and holds it through the entire miss path. No
concurrent get+null+compute+put without synchronization pattern found.
The mipmap/thumbnail cache follows the same pattern. No Thundering Herd
defect found.

View file

@ -0,0 +1,82 @@
# UNDF: (leave blank)
# CWE-312: darktable pwstorage backends log credentials verbatim when -d pwstorage
#
# darktable's password storage system (pwstorage) manages service credentials
# for cloud photo sharing services such as Piwigo. When a user saves their
# account to Piwigo, _piwigo_set_account() serializes their credentials into:
#
# {"server":"example.com","username":"user","password":"s3cr3t"}
#
# This JSON value flows into both pwstorage backends:
#
# 1. backend_kwallet.c dt_pwstorage_kwallet_set() line 368:
# dt_print(DT_DEBUG_PWSTORAGE, "...storing (%s, %s)", key, value);
# `value` is the full JSON blob including the plaintext password.
#
# 2. backend_kwallet.c dt_pwstorage_kwallet_get() line 555:
# dt_print(DT_DEBUG_PWSTORAGE, "...reading (%s, %s)", key, value);
# Same exposure on read.
#
# 3. backend_apple_keychain.c line 65:
# dt_print(DT_DEBUG_PWSTORAGE, "...storing (%s, %s)", key, value);
# Same JSON value including password.
#
# 4. backend_apple_keychain.c line 239:
# dt_print(DT_DEBUG_PWSTORAGE, "...reading (%s, %s)", server, json_data);
# json_data contains the reconstructed JSON with the password.
#
# The DT_DEBUG_PWSTORAGE flag is enabled at runtime with `darktable -d pwstorage`
# or `darktable -d all`. Debug logs go to stdout and optionally to the log file
# (~/.xsession-errors, journald, or a redirected terminal session). Any debug
# session — including those run to diagnose KWallet/Keychain connectivity — will
# expose the user's Piwigo password in plaintext in the terminal output.
#
# Lua scripts using darktable.password.save() are also affected: backend_kwallet
# and backend_apple_keychain receive the raw password string via the same path.
#
# Fix: log only the key (service name / username), never the value (which may
# contain the password). Replace `(%s, %s)` with `(%s, [REDACTED])` in all
# four dt_print calls.
#
# Severity: MEDIUM — requires the debug flag `darktable -d pwstorage` or
# `-d all` to be active. However, `-d all` is commonly used by users diagnosing
# OpenCL or other issues, which silently exposes credentials. Piwigo credentials
# are real username+password (not OAuth tokens), so exposure is direct account
# compromise.
#
--- a/src/common/pwstorage/backend_kwallet.c
+++ b/src/common/pwstorage/backend_kwallet.c
@@ -365,7 +365,7 @@ gboolean dt_pwstorage_kwallet_set(const backend_kwallet_context_t *context, con
while(g_hash_table_iter_next(&iter, &key, &value))
{
- dt_print(DT_DEBUG_PWSTORAGE, "[pwstorage_kwallet_set] storing (%s, %s)", (gchar *)key, (gchar *)value);
+ dt_print(DT_DEBUG_PWSTORAGE, "[pwstorage_kwallet_set] storing (%s, [REDACTED])", (gchar *)key);
gsize length;
gchar *new_key = char2qstring(key, &length);
@@ -552,7 +552,7 @@ GHashTable *dt_pwstorage_kwallet_get(const backend_kwallet_context_t *context,
dt_print(DT_DEBUG_PWSTORAGE,
- "[pwstorage_kwallet_get] reading (%s, %s)", (gchar *)key, (gchar *)value);
+ "[pwstorage_kwallet_get] reading (%s, [REDACTED])", (gchar *)key);
g_hash_table_insert(table, key, value);
}
--- a/src/common/pwstorage/backend_apple_keychain.c
+++ b/src/common/pwstorage/backend_apple_keychain.c
@@ -62,7 +62,7 @@ gboolean pwstorage_apple_keychain_set(const char *slot, GHashTable *table)
while(g_hash_table_iter_next(&iter, &key, &value))
{
- dt_print(DT_DEBUG_PWSTORAGE, "[pwstorage_apple_keychain_set] storing (%s, %s)", (gchar *) key, (gchar *) value);
+ dt_print(DT_DEBUG_PWSTORAGE, "[pwstorage_apple_keychain_set] storing (%s, [REDACTED])", (gchar *) key);
gchar *lbl = g_strconcat("darktable - ", slot, NULL);
@@ -236,7 +236,7 @@ GHashTable *pwstorage_apple_keychain_get(const char *slot)
dt_print(DT_DEBUG_PWSTORAGE,
- "[pwstorage_apple_keychain_get] reading (%s, %s)", server, json_data);
+ "[pwstorage_apple_keychain_get] reading (%s, [REDACTED])", server);
g_hash_table_insert(table, g_strdup(server), g_strdup(json_data));

View file

@ -0,0 +1,84 @@
# UNDF: (leave blank)
# CWE-407: modulegroups.c _lib_modulegroups_test_visible O(M*G*P) per module visibility check
#
# In src/libs/modulegroups.c, _lib_modulegroups_update_iop_visibility() iterates
# all IOP modules (length M) to determine whether each should be shown or hidden.
# For the DT_MODULEGROUP_NONE case, it calls _lib_modulegroups_test_visible() for
# each module. That function iterates all module groups (G) and for each group
# calls g_list_find_custom(gr->modules, module_op, _iop_compare) — a linear scan
# of the group's module list (length P).
#
# Total cost per UI refresh: O(M * G * P)
#
# Typical values: M=80 iop modules, G=8 groups, P=10 modules/group = 6,400
# g_strcmp0 calls per update. With the search text entry callback wired directly
# to _lib_modulegroups_update_iop_visibility, every keystroke triggers this.
# A user typing a 10-character search string incurs 64,000 string comparisons
# instead of 80.
#
# Fix: build a GHashTable (keyed on module op-name string) that covers all
# module names across all groups. Populate it once, then test_visible becomes
# a single g_hash_table_contains call — O(1).
# Invalidate/rebuild the hash table whenever d->groups is updated.
#
# The hash table can be stored in dt_lib_modulegroups_t and rebuilt in
# _lib_modulegroups_update (called whenever groups are loaded/saved).
#
# Severity: MEDIUM — 80x per-module savings; triggered on every text search
# keystroke, module group switch, and image load in the darkroom panel.
# Overhead ratio: ~80x at M=80 modules, G=8 groups, P=10 per group.
#
--- a/src/libs/modulegroups.c
+++ b/src/libs/modulegroups.c
@@ -107,6 +107,8 @@ typedef struct dt_lib_modulegroups_t
GList *groups;
GList *edit_groups;
+ /* Flat hash set of all module op-names visible in any group (for test_visible).
+ * Rebuilt whenever d->groups changes. */
+ GHashTable *visible_modules_set;
int current;
gboolean show_search;
@@ -277,12 +279,20 @@ static gboolean _lib_modulegroups_test_visible(dt_lib_module_t *self, gchar *mo
{
dt_lib_modulegroups_t *d = self->data;
- for(const GList *l = d->groups; l; l = g_list_next(l))
- {
- dt_lib_modulegroups_group_t *gr = l->data;
- if(g_list_find_custom(gr->modules, module, _iop_compare) != NULL)
- {
- return TRUE;
- }
- }
- return FALSE;
+ if(d->visible_modules_set)
+ return g_hash_table_contains(d->visible_modules_set, module);
+ /* Fallback to linear scan if hash not yet built (should not happen). */
+ for(const GList *l = d->groups; l; l = g_list_next(l))
+ {
+ dt_lib_modulegroups_group_t *gr = l->data;
+ if(g_list_find_custom(gr->modules, module, _iop_compare) != NULL)
+ return TRUE;
+ }
+ return FALSE;
}
+/* Rebuild the visible-modules hash set from d->groups.
+ * Call whenever groups are loaded, saved, or modified. */
+static void _lib_modulegroups_rebuild_visible_set(dt_lib_modulegroups_t *d)
+{
+ if(d->visible_modules_set)
+ g_hash_table_destroy(d->visible_modules_set);
+ d->visible_modules_set = g_hash_table_new(g_str_hash, g_str_equal);
+ for(const GList *l = d->groups; l; l = g_list_next(l))
+ {
+ dt_lib_modulegroups_group_t *gr = l->data;
+ for(const GList *m = gr->modules; m; m = g_list_next(m))
+ g_hash_table_add(d->visible_modules_set, m->data);
+ }
+}
+
/* Call _lib_modulegroups_rebuild_visible_set after every d->groups assignment. */
@@ -1458,2 +1472,3 @@ static void _lib_modulegroups_update(dt_lib_module_t *self, ...)
d->groups = res;
+ _lib_modulegroups_rebuild_visible_set(d);