Skip to content

fix: guard NULL part_ptr and license_list in slurmctld#202

Open
hossamazon wants to merge 1 commit into
SchedMD:masterfrom
hossamazon:fix-null-dereferencing
Open

fix: guard NULL part_ptr and license_list in slurmctld#202
hossamazon wants to merge 1 commit into
SchedMD:masterfrom
hossamazon:fix-null-dereferencing

Conversation

@hossamazon

Copy link
Copy Markdown

Summary

Two NULL-pointer crashes in slurmctld along the job-expansion path. Both are reachable on stock Slurm 25.11 without
exotic configuration: a job submitted with --exclusive=topo, or any caller that passes a job record whose
license_list was never allocated (jobs that never requested a license).

Bug 1: NULL part_ptr deref in node_mgr_make_node_blocked

File: src/slurmctld/node_mgr.c

The function gates its work behind IS_JOB_WHOLE_TOPO(job_ptr) and a job_resrcs null check, then unconditionally
dereferences job_ptr->part_ptr->topology_idx:

topology_g_whole_topo(tmp_bitmap, job_ptr->part_ptr->topology_idx);

The IS_JOB_WHOLE_TOPO macro (defined in src/common/job_record.h) is:

#define IS_JOB_WHOLE_TOPO(_X) \
    ((_X->details->whole_node & WHOLE_TOPO) || \
    (_X->part_ptr && (_X->part_ptr->flags & PART_FLAG_EXCLUSIVE_TOPO)))

The first disjunct returns true based purely on details->whole_node, with no check on part_ptr. So a job with
WHOLE_TOPO set on its details but no partition pointer attached passes the macro and segfaults on the following line.

WHOLE_TOPO is set by _job_create when the request specifies shared = JOB_SHARED_TOPO (i.e. --exclusive=topo).

Fix

Add an early return when part_ptr is NULL, after the existing guards:

if (... || !job_ptr->part_ptr)
    return;

Bug 2: NULL license_list deref in _valid_license_job_expansion

File: src/slurmctld/job_mgr.c

if (list_find_first_ro(job_ptr1->license_list, _find_hres, NULL) ||
    list_find_first_ro(job_ptr2->license_list, _find_hres, NULL))
    return false;

list_find_first_ro (src/common/list.c) calls _list_find_first_lock, which begins with xassert(l != NULL). In
debug builds this aborts the controller. In production builds xassert is compiled out and execution continues into
LIST_THREAD_LOCK(l, ...), which dereferences the NULL list pointer.

Either job record can have a NULL license_list (jobs that never requested a license), so the call site must guard.

Fix

Check license_list before each call:

if ((job_ptr1->license_list && ... ||
    (job_ptr2->license_list && ... ))
    return false;

Reproducer

Both bugs surface during the select_p_job_expand path:

  1. Submit two jobs where one has a dependency such that Slurm merges them (scontrol update job_id=N nodelist=...).
  2. Bug 1 fires when one of the jobs was submitted with --exclusive=topo and reaches node_mgr_make_node_blocked before part_ptr is wired up.
  3. Bug 2 fires when either job has no licenses requested (the common case) and the controller is built without xassert.

node_mgr_make_node_blocked dereferences job_ptr->part_ptr when calling topology_g_whole_topo, but the IS_JOB_WHOLE_TOPO macro can return true via (details->whole_node & WHOLE_TOPO) when part_ptr is still NULL. This segfaults slurmctld. Add an early return when part_ptr is NULL.

_valid_license_job_expansion passes license_list directly to list_find_first_ro, which xasserts on NULL. Debug builds abort and production builds (where xassert is compiled out) dereference NULL inside the lock path. Check license_list before each call.
@MeganD101

Copy link
Copy Markdown
Contributor

Hello,

Thank you for submitting this patch set. It does look like you are fixing two separate bugs in one commit. Could you please separate these fixes into their own commits? Please see the patch submission guidelines here:
https://github.com/SchedMD/slurm/blob/master/CONTRIBUTING.md#patch-submission

Additionally, Slurm uses clang-format and pre-commit to automatically manage the coding style. Could you please reformat your commits to match our coding style?

diff --git a/src/slurmctld/node_mgr.c b/src/slurmctld/node_mgr.c
index 5ca49e1916..102817f39f 100644
--- a/src/slurmctld/node_mgr.c
+++ b/src/slurmctld/node_mgr.c
@@ -4390,7 +4390,8 @@ extern void node_mgr_make_node_blocked(job_record_t *job_ptr, bool set)
        if (!IS_JOB_WHOLE_TOPO(job_ptr))
                return;
 
-       if (!job_ptr->job_resrcs || !job_ptr->job_resrcs->node_bitmap || !job_ptr->part_ptr)
+       if (!job_ptr->job_resrcs || !job_ptr->job_resrcs->node_bitmap ||
+           !job_ptr->part_ptr)
                return;
 
        tmp_bitmap = bit_copy(job_ptr->job_resrcs->node_bitmap);

Also it is preferred that commit message lines are wrapped at 76 characters.

The style guidelines can be found here:
https://github.com/SchedMD/slurm/blob/master/CONTRIBUTING.md#coding-guidelines

Lastly, your commits need to be sign off before they can be accepted. Please see the developer certification of origin guidelines here:
https://github.com/SchedMD/slurm/blob/master/CONTRIBUTING.md#developer-certificate-of-origin

Regards,
--Megan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants