On Sat, Jan 05, 2019 at 12:07:59PM -0700, Theo de Raadt wrote:
> + if (!*optarg)
>
> I despise this idiom. You are checking for a zero-length string.
> But you are hiding what is going on.
Because the value is used in many places. Some check for nullity, some
check for emptyness, others just strlcpy() it and leave error checks to
the ioctls.
> But why do you do it at every step??? Why not do it at the end
> after getopt?
How so?
This won't work:
while (ch = getopt(...)) {
switch (ch) {
...
}
/* breaks `-v' et al. unless I check `ch' again */
if (!*optarg)
errx(1, "empty -%c argument", ch);
}
And this is the same check but just scattered and deferred really:
while (...) {
...
}
...
if (ifaceopt && !*ifaceopt)
errx(1, "empty interface name");
...
memset(anchorname, 0, sizeof(anchorname));
if (anchoropt != NULL) {
if (!*anchoropt)
errx(1. "empty anchor name");
...
}
...
> Your diff feels wrong.
Well, it's an effective solution to multiple deficiencies that does not
require major code reshuffling.
I can work on further improvements but checking these tings as early as
possible is one step in the right direction, I think.
