[FFmpeg-devel] [PATCH 08/10] swscale/swscale_unscaled: Fix odd height with nv24_to_yuv420p_chroma()

[Ramiro Polla]

On Tue, Sep 24, 2024 at 3:35 PM Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Mon, Sep 23, 2024 at 12:42:22AM +0200, Ramiro Polla wrote:
> > Hi,
> >
> > On Mon, Sep 23, 2024 at 12:04 AM Michael Niedermayer
> > <michael at niedermayer.cc> wrote:
> > >
> > > Fixes: out of array read
> > > Fixes: 71726/clusterfuzz-testcase-ffmpeg_SWS_fuzzer-5876893532880896
> > >
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libswscale/swscale_unscaled.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c
> > > index dc1d5f35932..d403c953cc7 100644
> > > --- a/libswscale/swscale_unscaled.c
> > > +++ b/libswscale/swscale_unscaled.c
> > > @@ -230,6 +230,8 @@ static void nv24_to_yuv420p_chroma(uint8_t *dst1, int dstStride1,
> > >      const uint8_t *src2 = src + srcStride;
> > >      // average 4 pixels into 1 (interleaved U and V)
> > >      for (int y = 0; y < h; y += 2) {
> > > +        if (y + 1 == h)
> > > +            src2 = src1;
> > >          for (int x = 0; x < w; x++) {
> > >              dst1[x] = (src1[4 * x + 0] + src1[4 * x + 2] +
> > >                         src2[4 * x + 0] + src2[4 * x + 2]) >> 2;
> >
> > I would prefer to keep nv24_to_yuv420p_chroma() expecting height to be
> > a multiple of 2. We could add && !(c->srcH & 1) before selecting
> > nv24ToYuv420Wrapper.
>
> what advantage does this have ?

This would also have the benefit of keeping the function clearer, and
we wouldn't have to add special cases that might or might not be ok.
It would also make it easier to write simd code, since we wouldn't
have to worry about these special cases.

It is also easy to forget these corner cases, which leads to some
converters behaving differently in corner cases. For example, the
current yuyv422 to yuv420p scaler just doesn't output the last line if
it has an odd height. What is the correct fix? Should we convert the
last line differently like in this case? Should we duplicate the
second-to-last line? Should we just add a couple lines of code to
tweak the converter? Should we instead call this function with an even
height, and call another function for the last line?

IMO a converter to yuv420p with odd width or height shouldn't be an
unscaled converter, since we're effectively scaling the chroma planes
(and not just by a factor of two). In this case, the regular scaler
would be used.