Page MenuHomeFreeBSD

Multiple fixes to the NSS caching system
Needs ReviewPublic

Authored by david_crossfamilyweb.com on Jan 13 2023, 11:21 PM.
Referenced Files
Unknown Object (File)
Feb 17 2024, 4:42 AM
Unknown Object (File)
Jan 17 2024, 2:50 AM
Unknown Object (File)
Jan 14 2024, 9:35 AM
Unknown Object (File)
Dec 23 2023, 1:29 AM
Unknown Object (File)
Dec 1 2023, 6:37 AM
Unknown Object (File)
Nov 27 2023, 2:09 PM
Unknown Object (File)
Nov 25 2023, 1:05 AM
Unknown Object (File)
Oct 29 2023, 8:27 AM

Details

Reviewers
None
Group Reviewers
Src Committers
Summary

This addresses multiple issues in the NSS caching system, the hilights are:

  1. the caching loop prevention doesn't work, executables must be compiled -export-dynamic to have their symbols visible to dlsym() a. As a result of this not even working currently, take this opportunity to rename the symbol correctly.
  2. getgroupmembership() isn't cached at all if a NSS provider (ie LDAP) provides its own version, since that gets intercepted before the fallback is executed
  3. passwd and group lookups in nscd are not threadsafe a. I started to fix getservent as well before discovering this set uses thread-local-storage to fix it, I briefly debated migrating that to the passwd and group entries but feel this is a superior solution.
  4. negative cached lookups when using perform_lookups were not properly returned.
Test Plan

Tested quite extensively (as in I am actively using this) with nscd against nss_ldap with perform-actual-lookups for passwd and group. Should be tested with that off to ensure that I got the marshaling code correct in libc (aside, I debated unifying the marshaling/unmarshaling code with libc, but it would require a bit more of a rewrite as they use the code slightly differently)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Does this need any corresponding updates to manual pages, eg getpwent(3), getgrent(3), nsswitch.conf(5), or nscd(8)? My C reading skills aren't up to telling conclusively.

In D38047#865895, @pauamma wrote:

Does this need any corresponding updates to manual pages, eg getpwent(3), getgrent(3), nsswitch.conf(5), or nscd(8)? My C reading skills aren't up to telling conclusively.

Good question, I would say no, this does not affect WHAT is cached (in terms of databases), it fixes an oversight in the group database implementation and increases thread safety, there are no behavioral changes except for the oversight, which I would argue fixes a bug in incomplete group database handling,

Can you please run the libc pw* tests?

kyua test -k /usr/tests/lib/libc/nss/Kyuafile

Please see man 7 tests for more details.

lib/libc/gen/getgrent.c
245

The ellipses can be removed around the individual conditionals.

lib/libc/net/nsdispatch.c
133

It seems like this could break external compatibility with this symbol. Is there a reason why this should be renamed?

usr.sbin/nscd/agents/group.c
34–39

Could you please sort #include's?

73

(picking an instance)
style(9) says there should be a blank line before return.

98

The ellipses can be removed around the individual conditionals.

214

= vs space.

234–235

Can this be condensed to a single line?

242

Spurious space after memcpy.

243

= vs space.

245

Spurious space after memcpy.

378

This seems like it could result in a double free if the loop terminates and it hits line 392 or 401. Could you please defensively set group_storage = NULL; to avoid a potential future double free?

384

style(9): need a space between = and other portions of the line statement.

usr.sbin/nscd/agents/passwd.c
38

Please sort per style(9).

70–73

Is this indented correctly?

83

I have similar concerns about a double-free here as I did above.

260

style(9): missing space before/after =

270–272

style(9): line 271/272/300/301 should be indented with a hard tab and 4 spaces.

289

This should replace *result on line 208.

usr.sbin/nscd/query.c
806–813

Can this code be put on a single line and stay under 80 columns or be reindented similar to what I noted above about style(9).

pstef added inline comments.
usr.sbin/nscd/agents/group.c
73

Can you quote exactly? I can't find it. There is a mention of an optional blank "line at the beginning of functions with no local variables", but that's been optional for quite some time.

Please provide more diff context (diff -U999999).

usr.sbin/nscd/agents/group.c
92

size_t *buffer_size

lib/libc/net/nsdispatch.c
133

It could, however in this case because of the bug referenced in the Makefile (-export-dynamic), this check *never* worked for nscd. If there is some hypothetical other provider for the caching interface in libc for nameservice caching that was compiled correctly it could theoretically break that, however the caching system is relatively hardcoded for nscd, the socket_path is set to a #defined value of /var/run/nscd, so said hypothetical daemon would need to ninja its way deep into the intrenals of libc. Additionally the marshaling and unmarshaling functions would need to be in sync, in which case this entire change (or ANY change) to the nscd system could break things since it wouldn't understand new databases or types; because of this I would regard the NSCD interface as a private interface and we have a high degree of freedom to changea and improve it,

I'd like to at least make the name consistent with its purpose. Ideally, and eventually, I'd like to just make this a call into an internal/private method in libc to set a static variable to set behavior vs this kinda gross dynamic symbol reflection

usr.sbin/nscd/agents/group.c
234–235

This would cause the line to be just over 80 columns and wrap.

usr.sbin/nscd/agents/passwd.c
70–73

it appears to be on my terminal (using 8 space tabs everything lines up)

289

I don't follow this, there is only one occurance of *result in agents/passwd.c, (about line 71), and that function doesn't call this one?

usr.sbin/nscd/query.c
806–813

definitely not, the ; at the end of that line is already past 80 columns by a character.

Ran entire kyua test suite as follows:

$ kyua test -k /usr/tests/lib/libc/nss/Kyuafile
Results file id is usr_tests_lib_libc_nss.20230804-045215-435279
Results saved to /usr/home/david/.kyua/store/results.usr_tests_lib_libc_nss.20230804-045215-435279.db

96/96 passed (0 failed)

The .db file is 11Meg; I can attach as needed.

Cleaned up erroneous patches due to my tree being out of date with target.