Re: proftpd-dfsg: FTBFS on hurd-i386 (for review)
On Tue, 2014-04-15 at 19:21 +0200, Guillem Jover wrote:
> Hi!
>
> Another quick review.
>
> On Wed, 2014-04-09 at 15:19:25 +0200, Svante Signell wrote:
> > --- a/contrib/mod_exec.c
> > +++ b/contrib/mod_exec.c
> > @@ -735,6 +735,7 @@ static int exec_ssystem(cmd_rec *cmd, co
> >
> > if (fds >= 0) {
> > int buflen;
> > +#ifdef PIPE_BUF
>
> Why the different codepaths? This is prone to bitrot, and the dynamic
> allocated version should be good for the general case.
>
> > char buf[PIPE_BUF];
Removed the old code paths.
> > /* The child sent us something. How thoughtful. */
> > @@ -743,6 +744,19 @@ static int exec_ssystem(cmd_rec *cmd, co
> > memset(buf, '\0', sizeof(buf));
> >
> > buflen = read(exec_stdout_pipe[0], buf, sizeof(buf)-1);
> > +#else
> > + size_t len = fpathconf(exec_stdout_pipe[0], _PC_PIPE_BUF) - 1;
>
> Why the -1?
Changed, however -1 used in
buflen = read(exec_stdout_pipe[0], buf, len - 1);
as the original code.
> The indentation here is a bit messed up, also (w/o having seen further
> contenxt) I'd say you probably want to tell your editor to not use
> hardtabs on this codebase?
Fixed!
> Now, does exec_log() exit the program? Otherwise the subsequent use of
> buf will make the code segfault.
Fixed!?
> > --- a/contrib/mod_tls.c
> > +++ b/contrib/mod_tls.c
> > @@ -1765,10 +1765,18 @@ static int tls_exec_passphrase_provider(
> >
> > if (FD_ISSET(stderr_pipe[0], &readfds)) {
> > int stderrlen;
> > +#ifdef PIPE_BUF
> > char stderrbuf[PIPE_BUF];
> > -
>
> Unnecessary line removal.
Fixed!
> > memset(stderrbuf, '\0', sizeof(stderrbuf));
> > stderrlen = read(stderr_pipe[0], stderrbuf, sizeof(stderrbuf)-1);
> > +#else
> > + size_t len = fpathconf(stderr_pipe[0], _PC_PIPE_BUF) - 1;
> > + char *stderrbuf = malloc(len) + 1;
> > + if (stderrbuf == NULL)
> > + tls_log("malloc failed: %s", strerror(errno));
> > + memset(stderrbuf, '\0', len + 1);
> > + stderrlen = read(stderr_pipe[0], stderrbuf, len);
> > +#endif
>
> Same comments as with the exec_log() block.
Fixed!
> > --- a/contrib/mod_sftp/keys.c
> > +++ b/contrib/mod_sftp/keys.c
> > @@ -413,10 +413,20 @@ static int exec_passphrase_provider(serv
> >
> > if (FD_ISSET(stderr_pipe[0], &readfds)) {
> > int stderrlen;
> > +#ifdef PIPE_BUF
> > char stderrbuf[PIPE_BUF];
> >
> > memset(stderrbuf, '\0', sizeof(stderrbuf));
> > stderrlen = read(stderr_pipe[0], stderrbuf, sizeof(stderrbuf)-1);
> > +#else
> > + size_t len = fpathconf(stderr_pipe[0], _PC_PIPE_BUF) - 1;
> > + char *stderrbuf = malloc(len) + 1;
> > + if (stderrbuf == NULL)
> > + pr_log_pri(PR_LOG_ALERT, MOD_SFTP_VERSION ": Out of memory!");
> > + memset(stderrbuf, '\0', len + 1);
> > + stderrlen = read(stderr_pipe[0], stderrbuf, len);
> > +
> > +#endif
Fixed!
Thanks, an updated patch is attached. A remaining issue could be the
messages written, and the return codes. Maybe that is for upstream to
decide (if/when accepted)?
--- a/contrib/mod_exec.c
+++ b/contrib/mod_exec.c
@@ -735,14 +735,20 @@ static int exec_ssystem(cmd_rec *cmd, co
if (fds >= 0) {
int buflen;
- char buf[PIPE_BUF];
+
+ size_t len = fpathconf(exec_stdout_pipe[0], _PC_PIPE_BUF);
+ char *buf = malloc(len);
+ if (buf == NULL) {
+ exec_log("malloc failed: %s", strerror(errno));
+ return errno;
+ }
/* The child sent us something. How thoughtful. */
if (FD_ISSET(exec_stdout_pipe[0], &readfds)) {
- memset(buf, '\0', sizeof(buf));
+ memset(buf, '\0', len);
- buflen = read(exec_stdout_pipe[0], buf, sizeof(buf)-1);
+ buflen = read(exec_stdout_pipe[0], buf, len - 1);
if (buflen > 0) {
if (exec_opts & EXEC_OPT_SEND_STDOUT) {
@@ -789,9 +795,9 @@ static int exec_ssystem(cmd_rec *cmd, co
}
if (FD_ISSET(exec_stderr_pipe[0], &readfds)) {
- memset(buf, '\0', sizeof(buf));
+ memset(buf, '\0', len);
- buflen = read(exec_stderr_pipe[0], buf, sizeof(buf)-1);
+ buflen = read(exec_stdout_pipe[0], buf, len - 1);
if (buflen > 0) {
/* Trim trailing CRs and LFs. */
@@ -821,6 +827,7 @@ static int exec_ssystem(cmd_rec *cmd, co
}
}
}
+ free(buf);
}
res = waitpid(pid, &status, WNOHANG);
--- a/contrib/mod_tls.c
+++ b/contrib/mod_tls.c
@@ -1765,10 +1765,15 @@ static int tls_exec_passphrase_provider(
if (FD_ISSET(stderr_pipe[0], &readfds)) {
int stderrlen;
- char stderrbuf[PIPE_BUF];
- memset(stderrbuf, '\0', sizeof(stderrbuf));
- stderrlen = read(stderr_pipe[0], stderrbuf, sizeof(stderrbuf)-1);
+ size_t len = fpathconf(stderr_pipe[0], _PC_PIPE_BUF);
+ char *stderrbuf = malloc(len);
+ if (stderrbuf == NULL) {
+ tls_log("malloc failed: %s", strerror(errno));
+ return -1;
+ }
+ memset(stderrbuf, '\0', len + 1);
+ stderrlen = read(stderr_pipe[0], stderrbuf, len - 1);
if (stderrlen > 0) {
while (stderrlen &&
(stderrbuf[stderrlen-1] == '\r' ||
@@ -1785,6 +1790,7 @@ static int tls_exec_passphrase_provider(
": error reading stderr from '%s': %s",
tls_passphrase_provider, strerror(errno));
}
+ free(stderrbuf);
}
}
--- a/contrib/mod_sftp/keys.c
+++ b/contrib/mod_sftp/keys.c
@@ -413,10 +413,15 @@ static int exec_passphrase_provider(serv
if (FD_ISSET(stderr_pipe[0], &readfds)) {
int stderrlen;
- char stderrbuf[PIPE_BUF];
- memset(stderrbuf, '\0', sizeof(stderrbuf));
- stderrlen = read(stderr_pipe[0], stderrbuf, sizeof(stderrbuf)-1);
+ size_t len = fpathconf(stderr_pipe[0], _PC_PIPE_BUF);
+ char *stderrbuf = malloc(len);
+ if (stderrbuf == NULL) {
+ pr_log_pri(PR_LOG_ALERT, MOD_SFTP_VERSION ": Out of memory!");
+ return -1;
+ }
+ memset(stderrbuf, '\0', len);
+ stderrlen = read(stderr_pipe[0], stderrbuf, len - 1);
if (stderrlen > 0) {
while (stderrlen &&
(stderrbuf[stderrlen-1] == '\r' ||
@@ -433,6 +438,7 @@ static int exec_passphrase_provider(serv
": error reading stderr from '%s': %s",
passphrase_provider, strerror(errno));
}
+ free(stderrbuf);
}
}
Reply to: