Changeset View
Standalone View
head/libexec/ftpd/blacklist.c
| Property | Old Value | New Value |
|---|---|---|
| svn:eol-style | null | native \ No newline at end of property |
| svn:keywords | null | FreeBSD=%H \ No newline at end of property |
| svn:mime-type | null | text/plain \ No newline at end of property |
| /*- | |||||
| * Copyright (c) 2016 The FreeBSD Foundation | |||||
| * All rights reserved. | |||||
| * | |||||
| * This software was developed by Kurt Lidl under sponsorship from the | |||||
| * FreeBSD Foundation. | |||||
| * | |||||
| * Redistribution and use in source and binary forms, with or without | |||||
| * modification, are permitted provided that the following conditions | |||||
| * are met: | |||||
| * 1. Redistributions of source code must retain the above copyright | |||||
| * notice, this list of conditions and the following disclaimer. | |||||
| * 2. Redistributions in binary form must reproduce the above copyright | |||||
| * notice, this list of conditions and the following disclaimer in the | |||||
| * documentation and/or other materials provided with the distribution. | |||||
| * | |||||
| * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | |||||
| * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||
| * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||
| * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | |||||
| * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||
| * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||
| * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||
| * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||
| * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||
| * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||
| * SUCH DAMAGE. */ | |||||
| /* $FreeBSD$ */ | |||||
| #include <ctype.h> | |||||
| #include <stdarg.h> | |||||
| #include <stdlib.h> | |||||
| #include <unistd.h> | |||||
| #include "blacklist_client.h" | |||||
| #include <blacklist.h> | |||||
| static struct blacklist *blstate; | |||||
| void | |||||
| blacklist_init(void) | |||||
| { | |||||
| blstate = blacklist_open(); | |||||
| } | |||||
| void | |||||
| blacklist_notify(int action, int fd, char *msg) | |||||
| { | |||||
| if (blstate == NULL) | |||||
| blacklist_init(); | |||||
cem: Isn't this redundant? The only `blacklist_notify()` calls are clearly in the client session… | |||||
lidlAuthorUnsubmitted Not Done Inline ActionsNot 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, 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. lidl: Not exactly.
If the blacklist_init() fails for some reason, blstate will be NULL.
It will try… | |||||
cemUnsubmitted Not Done Inline Actions
Right—is there any reason to expect that blacklist_init fails the first time and doesn't fail again on the second try?
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?
I am not concerned about the performance cost, just the style / code smell. Thanks for doing this work! cem: > If the blacklist_init() fails for some reason, blstate will be NULL.
>
> It will try again to… | |||||
lidlAuthorUnsubmitted Not Done Inline ActionsI 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. lidl: I don't think it's particularly more likely to succeed calling the same initialization routine… | |||||
| if (blstate == NULL) | |||||
| return; | |||||
| (void)blacklist_r(blstate, action, fd, msg); | |||||
| } | |||||
Isn't this redundant? The only blacklist_notify() calls are clearly in the client session, which always initializes blacklist (right before the yyparse()).