On Thu, Jun 09, 2016 at 10:50:56PM +1100, Sylvain BERTRAND wrote:
> Hi,
>
> Introducing a new minimal and naive smtp server à la suckless: lnanosmtp
>
> https://github.com/sylware/lnanosmtp
> https://repo.or.cz/lnanosmtp.git
>
> cheers,
>
> --
> Sylvain
>
Dear Lord, it's been a while since I've seen such nice code make so
bafflingly bad design choices. Where to start?
1. The whole ulinux thing smacks a bit of NIH syndrome to me. And of
course, it kisses portability goodbye, but then that wasn't your goal at
all. With only i386 and AMD64 supported, I wonder why this isn't in
assembler.
2. You immediately block all signals on entry. That means:
- SIGINT, SIGQUIT, SIGTSTP, SIGTTOU, SIGTTIN: There is no way to
control the process from a terminal. You have to get another
terminal out to kill it.
- SIGSEGV, SIGILL, SIGBUS: Undefined behaviour if your code does
anything to warrant the sending of those signals.
- SIGCHLD: Zombies and no way of knowing about them. Why don't you
just ignore it?
- SIGXCPU, SIGXFSZ: No way to gracefully close on reaching ulimits.
And so on, and so forth. And for what? So you can handle signals using
signalfd. That would be good if you actually did that. However, that
only happens if the client never stalls out on you.
3. smtp_line_send() can't handle short writes, because the pointer that
is handed in as second argument to write() is never advanced, nor is the
unwritten part memmove()d to the front, as ridiculous as that would be.
4. Synchronous communication, no forking, no threading --> One client at
a time. So you're using epoll on the same two sockets all the time,
which means you might as well not have bothered.
Still, it could easily be salvaged: Drop all the setup code for the
server socket and make the code read from stdin and write to stdout.
Then this server can be run from inetd, or through any ucspi
implementation. That will also remove the glaring issue that the program
must be run as root and doesn't drop privileges even when it's done
doing privileged things.
5. Exiting at the drop of a hat: Your only error handling is to exit,
unless it was an expected error (usually EINTR). That's the opposite of
PHP, I guess, but that doesn't make it better.
6. You do know that if you used a C library, you'd have access to
strchr(), strcpy(), and the like, right? Because you seem to be having a
jolly good time rewriting those over and over again.
7. What's with the S_IFMT constants? You're binding hard to the syscall
interface, you can use 0666, it's not getting any less portable now.
8. You do know that C has more loop contructs than just the endless one,
right? Because I'm seeing a lot of endless loops that are basically
do-while loops, or counting loops... C has structured programming. Use
it! Please...
9. Oh wait, I see that you have strcpy(), you just don't use it.
Alrighty then.
And that was just what I saw in lnanosmtp.c. And I didn't check the
protocol.
Ciao,
Markus
