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

[Michael Niedermayer]

On Sat, Sep 28, 2024 at 02:01:33AM +0200, Michael Niedermayer wrote:
> On Wed, Sep 25, 2024 at 03:16:30PM +0200, Ramiro Polla wrote:
> > 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.
> 
> The SIMD code could support a subset of the c code
> 
> 
> > 
> > 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.
> 
> Iam not sure i understand correctly, but
> assuming a 99 lines 4:4:4 scaled to 99 lines 4:2:0
> the chroma here is not a simple "99 to 50 lines rescaling"
> the chroma samples must stay co-located in relation to the luma samples
> that can only happen with s 2:1 not a 99:50 rescaling
> but maybe i misunderstood what you meant

The fuzzer found another sample for this issue:
377735917/clusterfuzz-testcase-minimized-ffmpeg_SWS_fuzzer-6686071112400896

I still think my patch is fine, if someone else wants to disable the code
from being used for odd sizes thats not breaking the code, it just would
then not be used in some cases it could be used for.

If you prefer to only disable this for odd cases, please disable it.
I just want the bug fixed and not left open

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241202/dc3a8448/attachment.sig>