RFC: chop down fakeidentd?

Thomas Lundquist lists at zelow.no
Fri Jan 12 09:00:53 UTC 2007


On Thu, Jan 11, 2007 at 10:53:38PM +0100, Denis Vlasenko wrote:
> 
> Please take a cursory look as networking/fakeidentd.c.
> It's outright scary.

Believe me, it was worse.

I won't take credit for it being as good as it is, that was the work of
whoever ( I can't remember and he's been smart enough to not include
himself in the .c file :=).

> Why do we write pidfile?

it's nice behavior. you are the one wanting busybox to be run on a
desktop, so good behavior in an environment like that is the right thing
to do.

personally, I agree with you, rip it out but I also se no point
whatsoever in the desktop goal. busybox should be small and dedicated
embedded devices.

> Why do we setuid to "nobody"? What if I don't have "nobody"
> in the first place?

that should be an option. but I'd rather have it try to setuid nobody
while there are no nobody than not have it at all.

> Isn't it lame to rely on openlog() having open fd 1
> (when we closed all fd's except 0)? Yes, it is,
> but what standard guarantees that?
> 
> Why we have this MAXCONNS limit in fakeidentd,
> maybe we should do it in inetd instead so that ALL services
> get this protection from one copy of the code?

no. you can make it a libbb function but fakeident should be able to run
without any inetd. it will even without MAXCONNS but it's nice to have.

(MAXCONNS can be implemented via iptables anyway, so I won't cry if it's
gone, I just don't see the point.)

> Why we care to parse the request - we can use it verbatim:
> <- "6191, 23\r\n" - peer asks us "what user connected from your port 6191 to my port 23?"
> -> "6193, 23 : USERID : UNIX : nobody\r\n"
> 
> IOW: let's replace it with something like this:
> 
> int main(int argc, char **argv)
> {
>         char buf[128];
>         char *cur = buf;
>         int rem = sizeof(buf)-1;
> 
>         alarm(30);
>         while (1) {
>                 char *p;
>                 int sz = safe_read(0, cur, rem);
>                 if (sz < 0) return 1;
>                 rem -= sz;
>                 cur += sz;
>                 *cur = '\0';
>                 p = strpbrk(buf, "\r\n");
>                 if (p) {
>                         *p = '\0';
>                         break;
>                 }
>                 if (!rem || !sz)
>                         break;
>         }
>         printf("%s : USERID : UNIX : nobody\r\n", buf);
>         return 0;
> }
> 
> Objections?

You forgot the string it's supposed to be possible to change.
>From the existing code (fakeident_main):

    /* handle optional REPLY STRING */
    if (optind < argc)
        G.identuser = argv[optind];
    else
        G.identuser = nobodystr;

That's a feature we definately want to keep.

But I won't comment on the rest of the code, I've always thought it to
be a bit bloated.. but it is the least bloated and I didnt dare writing
my own.

So, if you keep the features, feel free to chop it down.


Thomas.



More information about the busybox mailing list