Page MenuHomeFreeBSD

Add blacklist support to ftpd
ClosedPublic

Authored by lidl on Jun 3 2016, 3:25 AM.

Details

Summary

Sponsored by: The FreeBSD Foundation

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

lidl updated this revision to Diff 17254.Jun 3 2016, 3:25 AM
lidl retitled this revision from to Add blacklist support to ftpd.
lidl updated this object.
lidl edited the test plan for this revision. (Show Details)
lidl added reviewers: rpaulo, emaste.
lidl set the repository for this revision to rS FreeBSD src repository.
rpaulo accepted this revision.Jun 3 2016, 5:52 AM
rpaulo edited edge metadata.
This revision is now accepted and ready to land.Jun 3 2016, 5:52 AM
This revision was automatically updated to reflect the committed changes.
cem added a subscriber: cem.Jun 3 2016, 3:53 PM
cem added inline comments.
head/libexec/ftpd/blacklist.c
50–51

Isn't this redundant? The only blacklist_notify() calls are clearly in the client session, which always initializes blacklist (right before the yyparse()).

lidl added inline comments.Jun 3 2016, 5:10 PM
head/libexec/ftpd/blacklist.c
50–51

Not exactly.

If the blacklist_init() fails for some reason, blstate will be NULL.

It will try again to blacklist_init(), and if it fails a second time,
blstate will still be NULL. In which case, it just gives up and
continues regular processing, forgoing any notification to blacklistd.

Given how ftpd in FreeBSD is structured, it's not necessary to explicitly call blacklist_init() just before the yyparse(). I put that in so there's a definitive point where it gets calls, no matter the code paths that are taken afterwards...

It just means in the common case, where blacklist_init() succeeds the first time, we do a couple of other pointer compares to NULL, which is pretty cheap.

cem added inline comments.Jun 3 2016, 5:13 PM
head/libexec/ftpd/blacklist.c
50–51

If the blacklist_init() fails for some reason, blstate will be NULL.
It will try again to blacklist_init(), and if it fails a second time,
blstate will still be NULL. In which case, it just gives up and
continues regular processing, forgoing any notification to blacklistd.

Right—is there any reason to expect that blacklist_init fails the first time and doesn't fail again on the second try?

Given how ftpd in FreeBSD is structured, it's not necessary to explicitly call blacklist_init() just before the yyparse(). I put that in so there's a definitive point where it gets calls, no matter the code paths that are taken afterwards...

Only because every notify call is checking for blstate, right? I don't understand why this daemon does it differently from all of the others. Either initialize it once where needed (like everything else does?), or drop the explicit initialization, no?

It just means in the common case, where blacklist_init() succeeds the first time, we do a couple of other pointer compares to NULL, which is pretty cheap.

I am not concerned about the performance cost, just the style / code smell. Thanks for doing this work!

lidl added inline comments.Jun 3 2016, 5:43 PM
head/libexec/ftpd/blacklist.c
50–51

I don't think it's particularly more likely to succeed calling the same initialization routine a second time.

The sshd changes (D5915) are similar to this one. (Feel free to comment on that review if you see anything.) If you have a daemon that is going to chroot() someplace, it obviously needs to call the library routine blacklist_open() before it does the chroot. (Or you need to arrange to have a socket endpoint in the chrooted area of the filesystem. Which blacklistd can do, but requires additional configuration.)

Anyway - the FreeBSD ftpd does do a chroot, but only *after* it has already verified that username/password (anonymous is handled a little differently).

The sendmail changes (which I need to move to a port) are also similar to this one. Most of the other daemons that I've converted are just spawned out of inetd, so there's no persistent daemon hanging around, so the blstate is just initialized just before use, and there will never be an attempt to reuse the blstate to communicate with the blacklistd process.

I will change this to have the explicit initialization, and take out the secondary attempt in blacklist_notify. I will also change it to use the symbolic name of STDIN_FILENO, rather than just '0' as it does now - someone else commented on that in some email.