On Wed, Aug 19, 2015 at 03:50:47PM +0200, Sebastien Marie wrote:
> On Wed, Aug 19, 2015 at 10:33:54AM +0200, Reyk Floeter wrote:
> >
> > In this case, "LibreSSL" was Theo who unintentionally broke ikectl.
> >
> > I attached a diff that generates new .cnf files by expanding the
> > variables in the source .cnf files and generating target .cnf files.
> > It works with both, ikeca.cnf and x508v3.cnf (ignore the warnings),
> > but you/we should install ikeca.cnf to /etc/ssl/ by default.
> >
> > There are more pending changes for ikectl (eg. from semarie@), but I'd
> > like to fix this first.
>
> for new code at least, you should check snprintf() return value for
> overflow. you could reuse the xsnprintf() code I sent previously if you
> want :)
>
I usually do one thing at a time. Yes, snprintf() doesn't check for
overflow but it is not adding any serious additional risk now - I
wanted to fix "basic operation" of ikectl first.
I'm not fond of adding x* functions (like xmalloc) but I agree that
the return values should be checked. But they should be checked
everywhere - I didn't forget about your diff. So maybe xsnprintf() is
OK in ikeca'c specific case.
Could you update and resend your ikectl diffs?
> and some others notes inline.
>
> > Index: ikeca.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ikectl/ikeca.c,v
> > retrieving revision 1.32
> > diff -u -p -u -p -r1.32 ikeca.c
> > --- ikeca.c 15 Aug 2015 04:47:28 -0000 1.32
> > +++ ikeca.c 19 Aug 2015 08:12:39 -0000
>
> [...]
>
> > @@ -489,6 +527,46 @@ fcopy(char *src, char *dst, mode_t mode)
> > }
> >
> > int
> > +fcopy_env(const char *src, const char *dst, mode_t mode)
> > +{
>
> returning int isn't useful: all errors are fatal and you always return 0
> value (which is also unused by caller).
>
Same here, I saw the useless return values in ikeca.c and just adopted
the style. It might sound crazy, but it is actually an invitation to
change it everywhere in a separate step (incl. fcopy()).
> > + int ofd = -1, i;
> > + u_int8_t buf[BUFSIZ];
> > + ssize_t r = -1, len;
> > + FILE *ifp = NULL;
> > + int saved_errno;
> > +
> > + if ((ifp = fopen(src, "r")) == NULL)
> > + err(1, "fopen %s", src);
> > +
> > + if ((ofd = open(dst, O_WRONLY|O_CREAT|O_TRUNC, mode)) == -1)
> > + goto done;
> > +
> > + while (fgets(buf, sizeof(buf), ifp) != 0) {
> > + for (i = 0; ca_env[i][0] != NULL; i++) {
> > + if (ca_env[i][1] == NULL)
> > + continue;
> > + expand_string(buf, sizeof(buf),
> > + ca_env[i][0], ca_env[i][1]);
> > + }
btw., the expand_string() return value is checked in the committed diff.
>
> something could go wrong here if fgets() partially read a normally expanded
> name:
>
> for example: file with `$ENV::CADB' inside
>
> one read:
> buf = "...$ENV::C"
> expand don't found `$ENV::CADB'
>
> next read
> buf = "ADB..."
>
> `$ENV::CADB' wouldn't be expanded
>
But how likely or valid is it that fgets() will return an incomplete
line from a .cnf file? It must be >BUFSIZ or a read from weird I/O
(maybe fuse or NFS) but fgets() would return NULL on I/O errors.
To be safe, I could a) check for feof() and ferror() and b) test if
the returned line includes a newline. Growing a buffer from multiple
lines doesn't seem to be necessary.
Reyk
> > + len = strlen(buf);
> > + if (write(ofd, buf, len) != len)
> > + goto done;
> > + }
> > +
> > + r = 0;
> > +
> > + done:
> > + saved_errno = errno;
> > + close(ofd);
> > + if (ifp != NULL)
> > + fclose(ifp);
> > + if (r == -1)
> > + errc(1, saved_errno, "open %s", dst);
> > +
> > + return (0);
> > +}
> > +
>
> --
> Sebastien Marie
--
