Re: proftpd-dfsg: FTBFS on hurd-i386 (for review)

[Guillem Jover <guillem@debian.org>]

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];
>  
>            /* 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?

> +	  char *buf = malloc(len) + 1;
> +	    if (buf == NULL)
> +	      exec_log("malloc failed: %s", strerror(errno));

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?

Now, does exec_log() exit the program? Otherwise the subsequent use of
buf will make the code segfault.

> --- 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.

>            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.

> --- 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

Same here.

Thanks,
Guillem