libnss_tcb: Match interfaces with NSS documentation. #31

besser82 · 2024-12-20T13:33:21Z

According to [1] the interfaces provided are of return-type 'enum nss_status', of which the _nss_database_getXXX_r() interfaces are mandated to take four parameters: a pointer to the result, a pointer to an additional buffer, the size of the buffer supplied, and a pointer to an integer to report error values (errnop). The returned status values are meant to be accompanied by a corresponding error value [2] passed through the errnop pointer.

[1] https://www.gnu.org/software/libc/manual/html_node/NSS-Module-Function-Internals.html
[2] https://www.gnu.org/software/libc/manual/html_node/NSS-Modules-Interface.html

besser82 · 2024-12-20T13:33:47Z

Last one for v1.3, promissed!

solardiz

I've made some comments based on skimming of the code, but also I need to review the spec and we need to review and cleanup our previous errno logic. This may be overly complicated with us separately maintaining errno and *__errnop.

Was the spec always this way or are we breaking compatibility with some legacy glibc?

ChangeLog

solardiz · 2024-12-20T16:27:55Z

libs/nss.c


-	if (asprintf(&file, TCB_FMT, name) < 0)
+	if (asprintf(&file, TCB_FMT, name) < 0) {
+		*__errnop = EAGAIN;


Should maybe be *__errnop = errno, because asprintf may set perhaps ENOMEM and it'd be right.

According to the specs EAGAIN looks correct to me, as NSS_STATUS_TRYAGAIN with errnop EAGAIN means: "One of the functions used ran temporarily out of resources or a service is currently not available."

The documentation says that in case of NSS_STATUS_TRYAGAIN only ERANGE has a special meaning, and everything else is as good as EAGAIN. The implementation agrees with the documentation, there is some special handling for ERANGE, but besides that all errno values are equal. In fact, __nss_getent_r returns EAGAIN in case of NSS_STATUS_TRYAGAIN regardless of errno value.

libs/nss.c

solardiz · 2024-12-20T16:30:42Z

libs/nss.c


 	switch (saved_errno) {
 	case 0:
+		if (!*result_buf_ptr) {
+			*__errnop = ENOENT;


Maybe EINVAL here? But is it a good idea to check for improper usage like that? It's typical for libc function to just crash when called with a NULL pointer where that is not allowed.

libs/nss.c

solardiz · 2024-12-20T16:32:02Z

libs/nss.c

 {
 	struct dirent *result;
 	off_t currpos;
 	int retval, saved_errno;

 	if (!tcbdir) {
 		tcbdir = opendir(TCB_DIR);
-		if (!tcbdir)
+		if (!tcbdir) {
+			*__errnop = ENOENT;


Maybe *__errnop = errno;.

NSS_STATUS_UNAVAIL with ENOENT means "A necessary input file cannot be found.", which is the case here; tcbdir cannot be opened.

solardiz · 2024-12-20T16:32:31Z

libs/nss.c

@@ -115,11 +142,13 @@ int _nss_tcb_getspent_r(struct spwd *__result_buf,
 			closedir(tcbdir);
 			errno = saved_errno;
 			tcbdir = NULL;
+			*__errnop = ENOENT;


I am not sure that our previous errno logic here was right, so maybe ENOENT is right here.

Yes, as we cannot iterate over tcbdir for unknown reasons, so we should indicate "A necessary input file cannot be found."

Given that in practice __errnop == &errno, assigning different values to errno and *__errnop doesn't make sense.

besser82 · 2024-12-20T18:16:03Z

Was the spec always this way or are we breaking compatibility with some legacy glibc?

The spec for the function prototype is unchanged since glibc-2.0.98 (~Oct 1998); the int *errnop parameter was added around this time, which was the reason for the libnss_$module soname has been bumped from 1 to 2.

See: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=c66dbe00b9c98bb9d1fc10c9007fdcfb98a59b73

include/attribute.h

besser82 · 2024-12-21T00:41:50Z

@solardiz I would be nice, if you could another look here; this rebase should address most of your previous comments.

solardiz · 2024-12-21T01:37:36Z

@solardiz I would be nice, if you could another look here; this rebase should address most of your previous comments.

Thanks. I will, but not right now. I want to take a look at the specs and hear from @ldv-alt his thoughts on whether we should continue setting errno as well (and if not, maybe removing that should be a separate commit).

According to [1] the interfaces provided are of return-type 'enum nss_status', of which the _nss_database_getXXX_r() interfaces are mandated to take four parameters: a pointer to the result, a pointer to an additional buffer, the size of the buffer supplied, and a pointer to an integer to report error values (errnop). The returned status values are meant to be accompanied by a corresponding error value [2] passed through the errnop pointer. [1] https://www.gnu.org/software/libc/manual/html_node/NSS-Module-Function-Internals.html [2] https://www.gnu.org/software/libc/manual/html_node/NSS-Modules-Interface.html Signed-off-by: Björn Esser <[email protected]>

ldv-alt · 2024-12-27T00:15:46Z

@solardiz I would be nice, if you could another look here; this rebase should address most of your previous comments.

Thanks. I will, but not right now. I want to take a look at the specs and hear from @ldv-alt his thoughts on whether we should continue setting errno as well (and if not, maybe removing that should be a separate commit).

I've checked the current glibc implementation of nss_files, and for me it looks like it's not quite consistent wrt setting errno.
For example, while its internal_getent sets errnop argument and strives not to clobber errno, internal_setent doesn't take errnop argument and sets errno, as result, _nss_files_get*_r may set either errno or errnop argument depending on the function where the error takes place. However, __nss_getent_r passes &errno as errnop argument, so the error ends up in errno anyway.

That is, we could make nss_tcb behavior wrt setting errno more consistent, but glibc doesn't care.

solardiz · 2024-12-27T04:00:05Z

That is, we could make nss_tcb behavior wrt setting errno more consistent, but glibc doesn't care.

OK, and what's your preference? Off the top of my head, our options may be:

Leave everything as-is (don't merge this PR).
Consistently set errno only (similar to option 1?)
Consistently set *errnop only, but let errno also be set/clobbered in places where this happens without extra effort by us.
Consistently set *errnop only and spend extra effort to preserve errno.
Merge this PR in whatever inconsistent state it may happen to be in. Looking at what's seen in patch context only, it appears that currently this PR consistently sets *errnop, but also sometimes spends extra effort to set errno.

ldv-alt · 2024-12-27T13:27:54Z

libs/nss.c

-	if (!f)
-		return errno == ENOENT ? NSS_STATUS_NOTFOUND : NSS_STATUS_UNAVAIL;
+	if (!f) {
+		/* $user/shadow not existing nor readable */
+		*__errnop = ENOENT;
+		return NSS_STATUS_UNAVAIL;
+	}


The original logic seems to be correct: ENOENT means that the shadow file doesn't exist, in that case NSS_STATUS_NOTFOUND (The requested entry is not available) should be returned. If the shadow file is not available for other reasons, than NSS_STATUS_UNAVAIL should be returned.

ldv-alt · 2024-12-27T13:36:34Z

libs/nss.c

-	retval = fgetspent_r(f, __result_buf, __buffer, __buflen, __result);
+	retval = fgetspent_r(f, __result_buf, __buffer,
+	    __buflen, &__result_buf);


Just to make sure, this is a bug fix: when fgetspent_r returns a nonzero value, it writes NULL at the address specified by its last argument. Before this change, that address used to be &errno.

ldv-alt · 2024-12-27T13:46:06Z

libs/nss.c

-	switch (saved_errno) {
+	/* real error number is retval from fgetspent_r(),
+	   by NSS spec errnop *MUST NOT* be set to 0 */
+	if (retval)
+		*__errnop = retval;
+
+	switch (retval) {


This is correct, the error code returned by fgetspent_r is currently either ENOENT or EAGAIN, it doesn't necessarily have to match the value of errno.

ldv-alt · 2024-12-27T13:50:38Z

libs/nss.c

-			errno = ENOENT;
+			errno = saved_errno;
 			tcbdir = NULL;
+			/* we have no more entries in tcbdir */
+			*__errnop = ENOENT;


ldv-alt · 2024-12-27T13:57:06Z

That is, we could make nss_tcb behavior wrt setting errno more consistent, but glibc doesn't care.

OK, and what's your preference? Off the top of my head, our options may be:

I think first of all we should update the function prototypes and fix the bug of passing __errnop to fgetspent_r.

After that we could fix other inconsistencies, keeping in the mind that in practice __errnop == &errno.

solardiz · 2024-12-27T16:10:28Z

I think first of all we should update the function prototypes and fix the bug of passing __errnop to fgetspent_r.

After that we could fix other inconsistencies, keeping in the mind that in practice __errnop == &errno.

So maybe 3 separate commits (fix bug, fix prototypes, start to use the passed *errnop and avoid inconsistencies)?

BTW, why the two underscores? I think that's inappropriate.

ldv-alt · 2024-12-27T21:43:33Z

BTW, why the two underscores? I think that's inappropriate.

I concur, identifiers with two leading underscores belong to the reserved namespace, it's not correct to introduce them here.

ldv-alt · 2024-12-27T21:56:13Z

I think first of all we should update the function prototypes and fix the bug of passing errnop to fgetspent_r.
After that we could fix other inconsistencies, keeping in the mind that in practice errnop == &errno.

So maybe 3 separate commits (fix bug, fix prototypes, start to use the passed *errnop and avoid inconsistencies)?

LGTM.

solardiz requested changes Dec 20, 2024

View reviewed changes

besser82 force-pushed the topic/besser82/nss_interface branch 4 times, most recently from 89c5d74 to ea61369 Compare December 20, 2024 23:45

solardiz reviewed Dec 21, 2024

View reviewed changes

include/attribute.h Outdated Show resolved Hide resolved

besser82 force-pushed the topic/besser82/nss_interface branch from ea61369 to 20c1bb2 Compare December 21, 2024 00:26

solardiz requested a review from ldv-alt December 21, 2024 01:33

besser82 force-pushed the topic/besser82/nss_interface branch from 20c1bb2 to b2ca446 Compare December 21, 2024 09:25

besser82 changed the title ~~libnss_tcb: Match interfaces with NSS documention.~~ libnss_tcb: Match interfaces with NSS documentation. Dec 21, 2024

besser82 force-pushed the topic/besser82/nss_interface branch from b2ca446 to a26fe47 Compare December 22, 2024 13:26

ldv-alt reviewed Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnss_tcb: Match interfaces with NSS documentation. #31

libnss_tcb: Match interfaces with NSS documentation. #31

besser82 commented Dec 20, 2024 •

edited

Loading

besser82 commented Dec 20, 2024

solardiz left a comment

solardiz Dec 20, 2024

besser82 Dec 20, 2024

ldv-alt Dec 27, 2024

solardiz Dec 20, 2024

solardiz Dec 20, 2024

besser82 Dec 21, 2024

solardiz Dec 20, 2024

besser82 Dec 21, 2024

ldv-alt Dec 27, 2024

besser82 commented Dec 20, 2024

besser82 commented Dec 21, 2024

solardiz commented Dec 21, 2024

ldv-alt commented Dec 27, 2024

solardiz commented Dec 27, 2024

ldv-alt Dec 27, 2024

ldv-alt Dec 27, 2024

ldv-alt Dec 27, 2024

ldv-alt Dec 27, 2024

ldv-alt commented Dec 27, 2024

solardiz commented Dec 27, 2024

ldv-alt commented Dec 27, 2024

ldv-alt commented Dec 27, 2024

libnss_tcb: Match interfaces with NSS documentation. #31

Are you sure you want to change the base?

libnss_tcb: Match interfaces with NSS documentation. #31

Conversation

besser82 commented Dec 20, 2024 • edited Loading

besser82 commented Dec 20, 2024

solardiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

besser82 commented Dec 20, 2024

besser82 commented Dec 21, 2024

solardiz commented Dec 21, 2024

ldv-alt commented Dec 27, 2024

solardiz commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldv-alt commented Dec 27, 2024

solardiz commented Dec 27, 2024

ldv-alt commented Dec 27, 2024

ldv-alt commented Dec 27, 2024

besser82 commented Dec 20, 2024 •

edited

Loading