[FFmpeg-devel] *** GMX Spamverdacht *** [PATCH] libavdevice/v4l2: fix of crash caused by assert

[Reimar Döffinger]

On Wed, Aug 13, 2014 at 07:04:01PM +0400, Dmitry Volyntsev wrote:
> From: Dmitry Volyntsev <xeioexception at gmail.com>
> 
> s->buffers_queued constantly decremented and not incremented
> in case of (s->frame_size > 0 && buf.bytesused != s->frame_size)
> condition (caught on long run capture of Logitech C310)
> ---
>  libavdevice/v4l2.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> index 64df0c7..25be95e 100644
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -510,9 +510,6 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt)
>          av_log(ctx, AV_LOG_ERROR, "Invalid buffer index received.\n");
>          return AVERROR(EINVAL);
>      }
> -    avpriv_atomic_int_add_and_fetch(&s->buffers_queued, -1);
> -    // always keep at least one buffer queued
> -    av_assert0(avpriv_atomic_int_get(&s->buffers_queued) >= 1);
>  
>      /* CPIA is a compressed format and we don't know the exact number of bytes
>       * used by a frame, so set it here as the driver announces it.
> @@ -527,6 +524,10 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt)
>          return AVERROR_INVALIDDATA;
>      }
>  
> +    avpriv_atomic_int_add_and_fetch(&s->buffers_queued, -1);
> +    // always keep at least one buffer queued
> +    av_assert0(avpriv_atomic_int_get(&s->buffers_queued) >= 1);

This is the wrong solution I think.
The problem is that the error path misses this part that all others do:
            if (v4l2_ioctl(s->fd, VIDIOC_QBUF, &buf) == 0)
                    avpriv_atomic_int_add_and_fetch(&s->buffers_queued, 1);
and this really needs to be factored out into an error handling path.
Especially since all these error paths fail to handle the ioctl failing
(e.g. they print no error message like the main path).
I also have some doubts that the ioctl error path actually does anything
sensible...
I guess the short version is: The whole error handling needs to be
reviewed and properly rewritten, adding hacks on top of it will probably
only make it unreliable and unmaintainable.