Issue 7126 - WARN - keys2idl - received NULL idl from index_read_ext_allids

[vashirov]

Bug Description:
Under high concurrency, occasional NULL IDL warnings were logged:

[29/Nov/2025:22:59:13.930441639 +0000] - WARN - keys2idl - received NULL idl from index_read_ext_allids, treating as empty set 
[29/Nov/2025:22:59:13.936543122 +0000] - WARN - keys2idl - this is probably a bug that should be reported 
[29/Nov/2025:22:59:13.947131944 +0000] - ERR - build_candidate_list - Database error -12795

Fix Description:
Ensure idl returned by index_read_ext_allids() is never NULL.

Fixes: #7126

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The new idl == NULL check inside the retry loop is redundant with the final idl == NULL check before return; you can likely drop the earlier allocation to keep the control flow and allocation behavior simpler while still guaranteeing a non-NULL return.
• When logging basetype in the new warning, consider guarding against a NULL pointer (or using a safe fallback string) to avoid introducing a potential NULL dereference in the error path.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `idl == NULL` check inside the retry loop is redundant with the final `idl == NULL` check before return; you can likely drop the earlier allocation to keep the control flow and allocation behavior simpler while still guaranteeing a non-NULL return.
- When logging `basetype` in the new warning, consider guarding against a NULL pointer (or using a safe fallback string) to avoid introducing a potential NULL dereference in the error path.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[droideck]

Also, there are a few more return NULL exists beside the main code path.
I don't have a strong opinion here though... So if you checked it under a high contention and the errors are gone, - I'm okay with that
Yet, it'd be nice to clean up the logic a bit as some of the function's callers don't do any NULL checks.

[tbordaz]

Agree with @droideck basetmp comments.
Except that LGTM

[vashirov]

Also, there are a few more return NULL exists beside the main code path. I don't have a strong opinion here though... So if you checked it under a high contention and the errors are gone, - I'm okay with that Yet, it'd be nice to clean up the logic a bit as some of the function's callers don't do any NULL checks.

I agree, index_read_ext_allids has IDList type, so we should always return a valid IDL.
I cleaned up other early exits to return empty IDL instead of NULL.

There are some checks for NULL IDL in

389-ds-base/ldap/servers/slapd/back-ldbm/db-mdb/mdb_ldif2db.c

Line 384 in 8fe7bfe

if (idl == NULL) {

and

389-ds-base/ldap/servers/slapd/back-ldbm/db-bdb/bdb_ldif2db.c

Line 493 in 8fe7bfe

if (idl == NULL) {

Since we changed the return type, I think we should adjust these as well, correct?

[droideck]

LGTM!

[droideck]

Just a note, the new commit message has a typo - idl_alloc(1) instead of idl_alloc(0).

Also a dead code in

389-ds-base/ldap/servers/slapd/back-ldbm/filterindex.c

Lines 1160 to 1164 in 8fe7bfe

	if (idl2 == NULL) {
	slapi_log_err(SLAPI_LOG_WARNING, "keys2idl", "received NULL idl from index_read_ext_allids, treating as empty set\n");
	slapi_log_err(SLAPI_LOG_WARNING, "keys2idl", "this is probably a bug that should be reported\n");
	idl2 = idl_alloc(0);
	}

[droideck]

Also, there are a few more return NULL exists beside the main code path. I don't have a strong opinion here though... So if you checked it under a high contention and the errors are gone, - I'm okay with that Yet, it'd be nice to clean up the logic a bit as some of the function's callers don't do any NULL checks.

I agree, index_read_ext_allids has IDList type, so we should always return a valid IDL. I cleaned up other early exits to return empty IDL instead of NULL.

There are some checks for NULL IDL in

389-ds-base/ldap/servers/slapd/back-ldbm/db-mdb/mdb_ldif2db.c

Line 384 in 8fe7bfe

if (idl == NULL) {

and

389-ds-base/ldap/servers/slapd/back-ldbm/db-bdb/bdb_ldif2db.c

Line 493 in 8fe7bfe

if (idl == NULL) {

Since we changed the return type, I think we should adjust these as well, correct?

I'm not fully sure...
I tend to lean toward the option to check like this:

if (*err == MDB_NOTFOUND) {

} else if (*err != 0) {

} else if (idl == NULL) {

IIUC, an empty ID list is perfectly valid for these cases and we can treat an empty-but-valid IDL (zero count, *err == 0) as a normal result.

@progier389 @tbordaz WDYT?