[FFmpeg-devel] [PATCH] ffserver: fix incorrect strlcpy usage

[Mark Harris]

On Fri, Nov 6, 2015 at 12:49 PM, Ganesh Ajjanagadde
<gajjanagadde at gmail.com> wrote:
> Somewhat ironic that this "safe" interface is actually being used
> unsafely here. This fixes the usage preventing potential null pointer
> dereference, where the old code was doubly broken: ctime can return
> NULL, and ctime can return an arbitrarily long buffer.
>
> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> ---
>  ffserver.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/ffserver.c b/ffserver.c
> index 526cbfc..108523e 100644
> --- a/ffserver.c
> +++ b/ffserver.c
> @@ -305,15 +305,18 @@ static void ffm_set_write_index(AVFormatContext *s, int64_t pos,
>      ffm->file_size = file_size;
>  }
>
> -static char *ctime1(char *buf2, int buf_size)
> +static char *ctime1(char *buf2, size_t buf_size)
>  {
>      time_t ti;
>      char *p;
>
>      ti = time(NULL);
>      p = ctime(&ti);
> -    av_strlcpy(buf2, p, buf_size);
> -    p = buf2 + strlen(p) - 1;
> +    if (!p) {
> +        *buf2 = '\0';
> +        return buf2;
> +    }
> +    p = buf2 + av_strlcpy(buf2, p, buf_size) - 1;
>      if (*p == '\n')
>          *p = '\0';
>      return buf2;

Ironically, this still doesn't handle a string that is too long for
the buffer.  "safe" indeed!  strlcpy (and av_strlcpy) returns the
length of the source, not the destination, so this will still access
and possibly write to memory beyond the end of buf2 when the string is
truncated.

It will also still access and possibly write to one byte before the
beginning of the buffer if the string is an empty string, although
that "should not happen".