Page MenuHomeFreeBSD

Unbreak nscd(8).
ClosedPublic

Authored by trasz on Oct 6 2018, 6:09 PM.

Details

Summary

Unbreak nscd(8). Without this change the CMSG gets truncated.
No idea how it worked before.

It also splits checks into separate "ifs", with a small tweak to debug.h
it makes it easier to see which one fails.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

trasz created this revision.Oct 6 2018, 6:09 PM
trasz updated this revision to Diff 48837.Oct 6 2018, 6:09 PM

Meh, wrong diff.

trasz edited the summary of this revision. (Show Details)Oct 6 2018, 6:10 PM
trasz added a subscriber: des.
trasz added reviewers: des, se, cy.Oct 7 2018, 12:07 PM
cy accepted this revision.Oct 7 2018, 12:33 PM

LGTM

This revision is now accepted and ready to land.Oct 7 2018, 12:33 PM
des requested changes to this revision.Oct 9 2018, 9:16 AM

The answer to “how it worked before” is that it didn't. It has never worked properly.

usr.sbin/nscd/query.c
189–201 ↗(On Diff #48837)

Please don't do this. Just fix the style issues in the original. I might even go one further and merge the four consistency checks with the recvmsg() call since they have the exact same error handling code. The fact that they're not really exactly the same because the macro uses __LINE__ is a bug, not a feature.

This revision now requires changes to proceed.Oct 9 2018, 9:16 AM
trasz marked an inline comment as done.Oct 9 2018, 2:28 PM
trasz added inline comments.
usr.sbin/nscd/query.c
189–201 ↗(On Diff #48837)

As it is now, it's useless. However, with one small diff (to be committed separately), it actually starts being kind of useful:

--- debug.c     (revision 339219)
+++ debug.c     (working copy)                                      
@@ -131,7 +131,7 @@
                for (i = 0; i < trace_level; ++i)
                        printf("\t");
                                                      
-               printf("<= %s\n", s);                   
+               printf("<= %s, %s: %d\n", s, f, l);
        }     
 }
trasz added a comment.Oct 9 2018, 2:29 PM

Having said that, the part in question doesn't belong here. So, I'll redo the diff.

trasz updated this revision to Diff 49119.Oct 13 2018, 10:28 PM
trasz edited the summary of this revision. (Show Details)

Remove unrelated parts of the diff, as suggested by des@.

trasz marked an inline comment as done.Oct 13 2018, 10:28 PM
des requested changes to this revision.Oct 30 2018, 12:31 PM

I believe this is correct (with or without the suggested changes), but send_credentials() in usr.sbin/nscd/nscdcli.c also needs to be fixed. It was not touched by rS158257, which introduced the bug you are fixing here while trying to fix another, so the bug is slightly different. It is supposed to be (almost) identical to lib/libc/net/nscachedcli.c; the latter is actually the former ported into libc.

Both files as well as usr.sbin/nscd/query.c have pervasive style issues, as I pointed out when nscd was first imported, but perhaps it's best to leave that for later.

usr.sbin/nscd/query.c
175 ↗(On Diff #49119)

This is technically correct, but I'd still rather spell it out explicitly with CMSG_SPACE (not CMSG_LEN).

186 ↗(On Diff #49119)

We should probably check cred_hdr.msg_controllen here.

This revision now requires changes to proceed.Oct 30 2018, 12:31 PM
trasz updated this revision to Diff 49856.Nov 1 2018, 12:08 AM

Fix stuff.

trasz updated this revision to Diff 49857.Nov 1 2018, 12:15 AM
trasz marked an inline comment as done.

One more length check.

trasz marked an inline comment as done.Nov 1 2018, 12:15 AM
des requested changes to this revision.Nov 1 2018, 8:15 AM

This version has a couple of regressions from the previous one. Remember that CMSG_SPACE and CMSG_LEN already include space for the header, plus alignment and padding. See CMSG_DATA(3) and sys/sys/socket.h.

usr.sbin/nscd/nscdcli.c
148 ↗(On Diff #49857)

struct cmsgcred

156 ↗(On Diff #49857)

struct cmsgcred

usr.sbin/nscd/query.c
175 ↗(On Diff #49857)

struct cmsgcred

This revision now requires changes to proceed.Nov 1 2018, 8:15 AM
trasz updated this revision to Diff 49880.Nov 1 2018, 1:13 PM

Use the right len.

trasz marked 3 inline comments as done.Nov 1 2018, 1:14 PM
des accepted this revision.Nov 1 2018, 2:11 PM
This revision is now accepted and ready to land.Nov 1 2018, 2:11 PM
This revision was automatically updated to reflect the committed changes.