Re: [PATCH] ossaudio: fix out of bounds write

[BALATON Zoltan]

On Wed, 8 Jul 2020, Philippe Mathieu-Daudé wrote:
> On 7/7/20 8:08 PM, Volker Rümelin wrote:
> > In function oss_read() a read error currently does not exit the
> > read loop. With no data to read the variable pos will quickly
> > underflow and a subsequent successful read overwrites memory
> > outside the buffer. This patch adds the missing break statement
> > to the error path of the function.
>
> Correct, but ...
>
> > To reproduce start qemu with -audiodev oss,id=audio0 and in the
> > guest start audio recording. After some time this will trigger
> > an exception.
> >
> > Fixes: 3ba4066d08 "ossaudio: port to the new audio backend api"
> >
> > Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> > ---
> >  audio/ossaudio.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/audio/ossaudio.c b/audio/ossaudio.c
> > index f88d076ec2..a7dcaa31ad 100644
> > --- a/audio/ossaudio.c
> > +++ b/audio/ossaudio.c
> > @@ -691,6 +691,7 @@ static size_t oss_read(HWVoiceIn *hw, void *buf, size_t len)
> >                             len, dst);
> >                  break;
> >              }
> > +            break;

Maybe it would be less confusing if you converted the switch(errno) to an 
if then you wouldn't have two senses of break; in close proximity. I was 
thinking something like

if (nread == -1) {
    if (errno != EINTR && errno != EAGAIN) {
        logerr();
    }
    break; /* from while, which is now clear */
}

> >          }
> >
> >          pos += nread;
>
> ... now pos += -1, then the size returned misses the last byte.

Don't you break from loop in if () above this so -1 is never added to pos 
after this patch? What happens for EINTR and EAGAIN? Now we break from the 
loop for those too, should it be continue; instead?

Regards,
BALATON Zoltan