Issue with systemd and sg(1), related to an assertion

[alejandro-colomar]

• lib/list.c: Use for loop instead of while loop #1470 (comment)

[alejandro-colomar]

shadow/lib/list.c

Lines 177 to 180 in 3e8c105

	bool is_on_list (char *const *list, const char *member)
	{
	assert (NULL != member);
	assert (NULL != list);

According to @stoeckmann , the assertion assert (NULL != list) can trigger, due to a systemd bug (systemd/systemd#40218).

I've opened this ticket to discuss what we should do here.

The call that triggers this is (I assume)

shadow/src/newgrp.c

Lines 145 to 148 in 3e8c105

	if ( (grp->gr_gid != pwd->pw_gid)
	&& !is_on_list (grp->gr_mem, pwd->pw_name)) {
	needspasswd = true;
	}

[stoeckmann]

The simplest solution would be to handle list == NULL here, arguing that a non-existing list does not contain a given member.

On the other hand, the sg->adm and sg->mem fields are supposed to be NULL-terminated string lists. Without this assertion, I wouldn't have noticed the bug (or incomplete implementation) in another project.

[alejandro-colomar]

The simplest solution would be to handle list == NULL here, arguing that a non-existing list does not contain a given member.

On the other hand, the sg->adm and sg->mem fields are supposed to be NULL-terminated string lists. Without this assertion, I wouldn't have noticed the bug (or incomplete implementation) in another project.

Yup; I think I prefer keeping the assertion.

Having a wider contract (accepting NULL input), as you say, can hide bugs, even if it is safer in the short term.

Having a narrow contract (aborting on NULL) might cause some issues (spurious abort(3)s), but eventually will lead to finding the bugs.

[alejandro-colomar]

I'll close for now.