From patchwork Tue Oct 17 17:20:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jorge Ramirez-Ortiz X-Patchwork-Id: 5597 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.161.90 with SMTP id m26csp427512jah; Tue, 17 Oct 2017 10:21:07 -0700 (PDT) X-Received: by 10.223.147.135 with SMTP id 7mr4586775wrp.237.1508260867285; Tue, 17 Oct 2017 10:21:07 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1508260867; cv=none; d=google.com; s=arc-20160816; b=q8pXFiZiGfUggM7Z7DJc6bThCq2lZvTSBDVb0er05YvZgCdc17Cn1xwnTBDiUk1VtF 3yR96lW2V/bEArgM9IWcFzlXyHeuYq+i2G/SY/JKQ6BTUgYLok5h1PlDSvPIJ4HONg/+ MhjWut70UMEEdMhCS/mPnicYrqYtFlqqq6l5Gr+v7NZdtkpitK3A/fkw9yBS0kASxPNr 4XvpJbVdz4QXfW86dEHwK125Q7vedscHaWXwl7V20u4WXrj15mSajMZWgjrxmiV/x9Fa 2gXPBas68VWopsVy/3yWMhl0Q7PEq5C2oFPiCvuY3pFPWrd/ln2wDkPF/mMMP3+l8eTc J6Kw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:content-transfer-encoding:mime-version:reply-to :list-subscribe:list-help:list-post:list-archive:list-unsubscribe :list-id:precedence:subject:message-id:date:to:from:dkim-signature :delivered-to:arc-authentication-results; bh=0HULm905fSTgmgv/dklmiH+rhl6MaJfLj5wUkcISCCU=; b=ptzKznzmJpKCkbs2DOMHcG3oZqdjbOQtuY/d7mcZoeeJlsVI4Ih8pOtWeN4ipvZY0Z hd28qEytGL9Ku4ghjnvKCjkXUqh/IWysioY8OUER9W1hFn0CQoZHBOpv8sgK0hMaSOFa OnWdo4r5dQllA6ZU1f6CUZcYcwk82Mlbgdi2PXZEnbx709Kq/E+D3tTNvIPOUnPfOI+M r5k3bx9azLF4mp0BAN5xiSKx4hWCxxCQgBw/dABKmAQfPR4E/KUmdm5/FhzJYcCOyLX8 ILI9pOQHZPaTRTomTuRxBZDhlPJpIjIkp7fuDsqN0NKdfv3wBUYnIc8djKtD2aX7+5Bm UJTA== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=ir0AhQp5; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id c197si7767217wme.136.2017.10.17.10.21.04; Tue, 17 Oct 2017 10:21:07 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=ir0AhQp5; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D8D39689E4D; Tue, 17 Oct 2017 20:20:58 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr0-f179.google.com (mail-wr0-f179.google.com [209.85.128.179]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id B056B689D66 for ; Tue, 17 Oct 2017 20:20:52 +0300 (EEST) Received: by mail-wr0-f179.google.com with SMTP id q42so2470223wrb.7 for ; Tue, 17 Oct 2017 10:20:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:subject:date:message-id; bh=Qn5n7muad1MOpMSauO7li1o2iWoZqX29eNAaLJvGJl4=; b=ir0AhQp5oAXVLT/+7j4H9tLzkaulVTDKcpfUPKYACzc/Er8/wuicdRgRFpdeAxYGTB XS9YXi/Zg9K2APXG2NENf4wC06pwcPFArm8v848Kq/mhOTfLmOM4zqWUOV62AKTxXCQw H+K2OkZYj4GOsqoZr6gCzb4e7+dQ32FwxwBPQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=Qn5n7muad1MOpMSauO7li1o2iWoZqX29eNAaLJvGJl4=; b=i8wUhtdagOPkzd/lV3tOBvyt11BhPxDcSXjDF7GZ2PWGb33VsHye8gTP3iu+75nNre pfdkoH8oDNWwm3WXVh1jDH6zEOxNSach1QU7RhDB5nBiPeQdL9aodnGOf6ktNw7eJrQh wgHC77RPTTZ7bv2J5uIm1TMo3N2R8EbWuIi8KhEvfpSq2wMBGa8y+UiFLCEmiKJH3jKB byTP2niswTeeCruIhC7OolMMojpcIlfvHPnPp95tiYp4MK3S9Up5Dblwobtu1lUcdVKh YWQBd/BT6f5tc9uAxXdzUXL4a0HcWFI0qxeSP11eSMnrjlj+Xze/SKd1r3adnRYv5qNW 7YdA== X-Gm-Message-State: AMCzsaV9RGb93K2/Syg/ZPz621/by16YQ+rgye1lE0b2TpXgM1gEoLrt uv4VxvAXL/9EItISIcMFV0E1Tg== X-Google-Smtp-Source: ABhQp+RlbUiBmXwmirdITjxYNWF12ig3n01peoYPdEbjSb4qIS+7MEC4N07BoaDe3LgnKIuXPhzM7Q== X-Received: by 10.223.139.82 with SMTP id v18mr4629545wra.55.1508260856787; Tue, 17 Oct 2017 10:20:56 -0700 (PDT) Received: from igloo.80.58.61.254 (216.red-95-126-110.staticip.rima-tde.net. [95.126.110.216]) by smtp.gmail.com with ESMTPSA id 5sm15893594wrt.59.2017.10.17.10.20.55 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 17 Oct 2017 10:20:55 -0700 (PDT) From: Jorge Ramirez-Ortiz To: jorge.ramirez-ortiz@linaro.org, ffmpeg-devel@ffmpeg.org, sw@jkqxz.net Date: Tue, 17 Oct 2017 19:20:50 +0200 Message-Id: <1508260850-6914-1-git-send-email-jorge.ramirez-ortiz@linaro.org> X-Mailer: git-send-email 2.7.4 Subject: [FFmpeg-devel] [PATCH] avcodec/v4l2: fix access to priv_data after codec close. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches MIME-Version: 1.0 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" A user can close the codec while keeping references to some of the capture buffers. When the codec is closed, the structure that keeps the contexts, state and the driver file descriptor is freed. Since access to some of the elements in that structure is required to properly release the memory during the buffer unref callbacks, the following commit makes sure the unref callback accesses valid memory. This commit was tested with valgrind: $ valgrind ffmpeg_g -report -threads 1 -v 55 -y -c:v h264_v4l2m2m \ -i video.h264 -an -frames:v 100 -f null - --- libavcodec/v4l2_buffers.c | 17 ++++++++++++++++- libavcodec/v4l2_buffers.h | 6 ++++++ libavcodec/v4l2_m2m.c | 29 ++++++++++++++++++++++++----- libavcodec/v4l2_m2m_dec.c | 2 +- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c index ba70c5d..80e65ca 100644 --- a/libavcodec/v4l2_buffers.c +++ b/libavcodec/v4l2_buffers.c @@ -205,9 +205,24 @@ static enum AVColorTransferCharacteristic v4l2_get_color_trc(V4L2Buffer *buf) static void v4l2_free_buffer(void *opaque, uint8_t *unused) { V4L2Buffer* avbuf = opaque; - V4L2m2mContext *s = buf_to_m2mctx(avbuf); + V4L2m2mContext *s = avbuf->m2m ? avbuf->m2m : buf_to_m2mctx(avbuf); atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel); + + if (avbuf->m2m) { + if (!atomic_load(&s->refcount)) { + /* unmmap and free the associated buffers */ + ff_v4l2_context_release(&s->capture); + + /* close the v4l2 driver */ + close(s->fd); + + /* release the duplicated m2m context */ + av_freep(&s); + } + return; + } + if (s->reinit) { if (!atomic_load(&s->refcount)) sem_post(&s->refsync); diff --git a/libavcodec/v4l2_buffers.h b/libavcodec/v4l2_buffers.h index e28a4a6..d774480 100644 --- a/libavcodec/v4l2_buffers.h +++ b/libavcodec/v4l2_buffers.h @@ -41,6 +41,12 @@ typedef struct V4L2Buffer { /* each buffer needs to have a reference to its context */ struct V4L2Context *context; + /* when the codec is closed while the user has buffer references we + * still need access to context data (this is a pointer to a duplicated + * m2m context created during the codec close function). + */ + struct V4L2m2mContext *m2m; + /* keep track of the mmap address and mmap length */ struct V4L2Plane_info { int bytesperline; diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c index 1d7a852..91b1bbb 100644 --- a/libavcodec/v4l2_m2m.c +++ b/libavcodec/v4l2_m2m.c @@ -313,8 +313,8 @@ error: int ff_v4l2_m2m_codec_end(AVCodecContext *avctx) { - V4L2m2mContext* s = avctx->priv_data; - int ret; + V4L2m2mContext *m2m, *s = avctx->priv_data; + int i, ret; ret = ff_v4l2_context_set_status(&s->output, VIDIOC_STREAMOFF); if (ret) @@ -325,12 +325,31 @@ int ff_v4l2_m2m_codec_end(AVCodecContext *avctx) av_log(avctx, AV_LOG_ERROR, "VIDIOC_STREAMOFF %s\n", s->capture.name); ff_v4l2_context_release(&s->output); + sem_destroy(&s->refsync); - if (atomic_load(&s->refcount)) - av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end leaving pending buffers\n"); + if (atomic_load(&s->refcount)) { + av_log(avctx, AV_LOG_DEBUG, "ff_v4l2m2m_codec_end, referenced buffers %d\n", atomic_load(&s->refcount)); + + /* We are about to free the private data while the user still has references to the buffers. + * This means that when the user releases those buffers, the v4l2_free_buffer callback will be pointing to free'd memory. + * Duplicate the m2m context and update the buffers. + */ + m2m = av_mallocz(sizeof(*m2m)); + if (m2m) { + memcpy(m2m, s, sizeof(V4L2m2mContext)); + for (i = 0; i < s->capture.num_buffers; i++) + s->capture.buffers[i].m2m = m2m; + + return 0; + } + + /* on ENOMEM let's at least close the v4l2 driver and release the capture memory + * so the driver is left in a healthy state. + */ + av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end, referenced buffers not accessible\n"); + } ff_v4l2_context_release(&s->capture); - sem_destroy(&s->refsync); /* release the hardware */ if (close(s->fd) < 0 ) diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c index 958cdc5..f88f819 100644 --- a/libavcodec/v4l2_m2m_dec.c +++ b/libavcodec/v4l2_m2m_dec.c @@ -38,7 +38,7 @@ static int v4l2_try_start(AVCodecContext *avctx) V4L2m2mContext *s = avctx->priv_data; V4L2Context *const capture = &s->capture; V4L2Context *const output = &s->output; - struct v4l2_selection selection; + struct v4l2_selection selection = { 0 }; int ret; /* 1. start the output process */