From patchwork Wed Jul 1 18:14:13 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Kim X-Patchwork-Id: 20764 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 9AF1A44B286 for ; Wed, 1 Jul 2020 21:14:33 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 7464968A8C7; Wed, 1 Jul 2020 21:14:33 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-ej1-f42.google.com (mail-ej1-f42.google.com [209.85.218.42]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id B5EEB6883C8 for ; Wed, 1 Jul 2020 21:14:26 +0300 (EEST) Received: by mail-ej1-f42.google.com with SMTP id o18so21512086eje.7 for ; Wed, 01 Jul 2020 11:14:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:from:date:message-id:subject:to; bh=RgAM5/hZtRsharVTEQX6Zt7ugstMNw3huzCz0GwRfm0=; b=lCReX/SnkhFN04QCCr2B2T9jcbYW+8pY0c9aqGDpf/4sLDnHv7ShXS30v5cWtk3XTh COHpUoGSHyzgcOJpC7viWG2gFK9DG5FmngtQHtA0PMOy+0W9+7RhrwBxO4HVEeXh/vsV 4AQbg552Zdn1HPntsgsS1YCNW+nQv5KCHIhNEeWbfZOLeiXP8HBWaID0HQD5K+hTdSWH DlwiwFItsx1VSt5RmV0zPz/qTP6bDiGtUuoWPAN0D9j/g93I+wERTNGFaoBShjz8MWCi puiwFk1krPPDoDZQJaJeKSGu6Lja/B3fGwOgetyDXVWf27/jrRl4+lVLv28RcDDZMBvJ KYBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=RgAM5/hZtRsharVTEQX6Zt7ugstMNw3huzCz0GwRfm0=; b=cK8jVPXq61SPZ0bP8ch6NtkRpngwl+qXhIeJuGu764gESOfcbx38nn3a1PjEPZyx3P dLcLljHqJBXPxw00yX7qpJ8IUezLXHAqwWD9iZMxDYswyZAUKKetEX9tbKuOEyJxxXNJ 89b92vUEbCYfYNJoubICbXN0a1Gr/Pz9+zbjlXRzHw2tvwsdTZGAHAenNeHlZLY+qQQi e3bXYOW93NMvxDNGO5DodwaHMEHwta9Ts18bi5uwXlHzkvDW/s6JtVXWW1SUQ17afmz0 leEbaJ7IzU5JNJv65EhmjcMLk3g3rcnfrJ01cBABnKXzXzUvxkpU3koKeRxccf3Bkz2O ZI8w== X-Gm-Message-State: AOAM5338q36ZqWUtlcqAjwF2bFBaw+qHPStVDAaj7ilF+5MlijActDh1 txxbTb0Hq2bl1gIFLgtevvCZ2QbIZC0ib+rzRdm6J3dExYU= X-Google-Smtp-Source: ABdhPJzLERn6a9DTQuD6/CWiU7ljpZHL7kqRW2Xv2RSn6Q96dKcNuRipjKMY3FvTsFNHsd6cKJTbSRPwCZSa7F6KOjU= X-Received: by 2002:a17:906:2b0e:: with SMTP id a14mr23380681ejg.459.1593627265345; Wed, 01 Jul 2020 11:14:25 -0700 (PDT) MIME-Version: 1.0 From: Brian Kim Date: Wed, 1 Jul 2020 11:14:13 -0700 Message-ID: To: ffmpeg-devel@ffmpeg.org X-Content-Filtered-By: Mailman/MimeDel 2.1.20 Subject: [FFmpeg-devel] libavutil/imgutils: UBSan nullptr-with-offset in av_image_fill_pointer 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" While running under Clang's UndefinedBehaviorSanitizer, I found a few places where av_image_fill_pointers is called before buffers for the image are allocated, so ptr is passed in as NULL. This leads to (currently harmless) UB when the plane sizes are added to the null pointer, so I was wondering if there was interest in avoiding it? I've attached a patch to expose some extra utilities and avoid passing in the null pointer, but is this an appropriate way to work around it? Thank you, Brian Subject: [PATCH] libavutil/imgutils: add utility to get plane sizes This utility helps avoid undefined behavior when doing things like checking how much memory we need to allocate for an image before we have allocated a buffer. --- libavcodec/decode.c | 11 +++------- libavutil/frame.c | 9 ++++---- libavutil/imgutils.c | 49 ++++++++++++++++++++++++++++++++------------ libavutil/imgutils.h | 22 ++++++++++++++++++++ 4 files changed, 65 insertions(+), 26 deletions(-) diff --git a/libavcodec/decode.c b/libavcodec/decode.c index de9c079f9d..cd0424b467 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1471,9 +1471,8 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) switch (avctx->codec_type) { case AVMEDIA_TYPE_VIDEO: { - uint8_t *data[4]; int linesize[4]; - int size[4] = { 0 }; + int size[4]; int w = frame->width; int h = frame->height; int tmpsize, unaligned; @@ -1494,17 +1493,13 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) unaligned |= linesize[i] % pool->stride_align[i]; } while (unaligned); - tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h, - NULL, linesize); + tmpsize = av_image_fill_plane_sizes(size, avctx->pix_fmt, h, + linesize); if (tmpsize < 0) { ret = tmpsize; goto fail; } - for (i = 0; i < 3 && data[i + 1]; i++) - size[i] = data[i + 1] - data[i]; - size[i] = tmpsize - (data[i] - data[0]); - for (i = 0; i < 4; i++) { pool->linesize[i] = linesize[i]; if (size[i]) { diff --git a/libavutil/frame.c b/libavutil/frame.c index 9884eae054..3abb1f12d5 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -212,6 +212,7 @@ void av_frame_free(AVFrame **frame) static int get_video_buffer(AVFrame *frame, int align) { const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format); + int size[4]; int ret, i, padded_height; int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align); @@ -239,8 +240,8 @@ static int get_video_buffer(AVFrame *frame, int align) } padded_height = FFALIGN(frame->height, 32); - if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height, - NULL, frame->linesize)) < 0) + if ((ret = av_image_fill_plane_sizes(size, frame->format, padded_height, + frame->linesize)) < 0) return ret; frame->buf[0] = av_buffer_alloc(ret + 4*plane_padding); @@ -249,9 +250,7 @@ static int get_video_buffer(AVFrame *frame, int align) goto fail; } - if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height, - frame->buf[0]->data, frame->linesize)) < 0) - goto fail; + av_image_fill_pointers_from_sizes(frame->data, size, frame->buf[0]->data); for (i = 1; i < 4; i++) { if (frame->data[i]) diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c index 7f9c1b632c..7045a9df65 100644 --- a/libavutil/imgutils.c +++ b/libavutil/imgutils.c @@ -108,26 +108,25 @@ int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int wi return 0; } -int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int height, - uint8_t *ptr, const int linesizes[4]) +int av_image_fill_plane_sizes(int size[4], enum AVPixelFormat pix_fmt, int height, + const int linesizes[4]) { - int i, total_size, size[4] = { 0 }, has_plane[4] = { 0 }; + int i, total_size, has_plane[4] = { 0 }; const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt); - memset(data , 0, sizeof(data[0])*4); + memset(size , 0, sizeof(size[0])*4); if (!desc || desc->flags & AV_PIX_FMT_FLAG_HWACCEL) return AVERROR(EINVAL); - data[0] = ptr; if (linesizes[0] > (INT_MAX - 1024) / height) return AVERROR(EINVAL); size[0] = linesizes[0] * height; if (desc->flags & AV_PIX_FMT_FLAG_PAL || desc->flags & FF_PSEUDOPAL) { - data[1] = ptr + size[0]; /* palette is stored here as 256 32 bits words */ - return size[0] + 256 * 4; + size[1] = 256 * 4; /* palette is stored here as 256 32 bits words */ + return size[0] + size[1]; } for (i = 0; i < 4; i++) @@ -136,7 +135,6 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei total_size = size[0]; for (i = 1; i < 4 && has_plane[i]; i++) { int h, s = (i == 1 || i == 2) ? desc->log2_chroma_h : 0; - data[i] = data[i-1] + size[i-1]; h = (height + (1 << s) - 1) >> s; if (linesizes[i] > INT_MAX / h) return AVERROR(EINVAL); @@ -149,6 +147,31 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei return total_size; } +void av_image_fill_pointers_from_sizes(uint8_t *data[4], int size[4], uint8_t *ptr) +{ + int i; + + memset(data , 0, sizeof(data[0])*4); + + data[0] = ptr; + for (i = 1; i < 4 && size[i - 1] > 0; i++) + data[i] = data[i - 1] + size[i - 1]; +} + +int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int height, + uint8_t *ptr, const int linesizes[4]) { + int size[4]; + int ret; + + ret = av_image_fill_plane_sizes(size, pix_fmt, height, linesizes); + if (ret < 0) + return ret; + + av_image_fill_pointers_from_sizes(data, size, ptr); + + return ret; +} + int avpriv_set_systematic_pal2(uint32_t pal[256], enum AVPixelFormat pix_fmt) { int i; @@ -194,6 +217,7 @@ int av_image_alloc(uint8_t *pointers[4], int linesizes[4], { const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt); int i, ret; + int size[4]; uint8_t *buf; if (!desc) @@ -207,19 +231,18 @@ int av_image_alloc(uint8_t *pointers[4], int linesizes[4], for (i = 0; i < 4; i++) linesizes[i] = FFALIGN(linesizes[i], align); - if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, NULL, linesizes)) < 0) + if ((ret = av_image_fill_plane_sizes(size, pix_fmt, h, linesizes)) < 0) return ret; buf = av_malloc(ret + align); if (!buf) return AVERROR(ENOMEM); - if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, buf, linesizes)) < 0) { - av_free(buf); - return ret; - } + av_image_fill_pointers_from_sizes(pointers, size, buf); + if (desc->flags & AV_PIX_FMT_FLAG_PAL || (desc->flags & FF_PSEUDOPAL && pointers[1])) { avpriv_set_systematic_pal2((uint32_t*)pointers[1], pix_fmt); if (align < 4) { av_log(NULL, AV_LOG_ERROR, "Formats with a palette require a minimum alignment of 4\n"); + av_free(buf); return AVERROR(EINVAL); } } diff --git a/libavutil/imgutils.h b/libavutil/imgutils.h index 5b790ecf0a..cbdef12928 100644 --- a/libavutil/imgutils.h +++ b/libavutil/imgutils.h @@ -67,6 +67,28 @@ int av_image_get_linesize(enum AVPixelFormat pix_fmt, int width, int plane); */ int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int width); +/** + * Fill plane sizes for an image with pixel format pix_fmt and height height. + * + * @param size the array to be filled with the size of each image plane + * @param linesizes the array containing the linesize for each + * plane, should be filled by av_image_fill_linesizes() + * @return the size in bytes required for the image buffer, a negative + * error code in case of failure + */ +int av_image_fill_plane_sizes(int size[4], enum AVPixelFormat pix_fmt, int height, + const int linesizes[4]); + +/** + * Fill plane data pointers for an image with plane sizes size. + * + * @param data pointers array to be filled with the pointer for each image plane + * @param size the array containing the size of each plane, should be filled + * by av_image_fill_plane_sizes() + * @param ptr the pointer to a buffer which will contain the image + */ +void av_image_fill_pointers_from_sizes(uint8_t *data[4], int size[4], uint8_t *ptr); + /** * Fill plane data pointers for an image with pixel format pix_fmt and * height height.