[MPlayer-dev-eng] Too big buffer in "libmpcodecs/ad_pcm.c"?

[Uoti Urpala]

On Mon, 2006-08-14 at 20:05 +0200, Reimar Döffinger wrote:
> > above. It's enough to assume that maxlen >= minlen + audio_out_minsize,
> > and that should be guaranteed by the calling functions.
> 
> That's one of the biggest problems I have with MPlayer. You say this
> should be guaranteed by the calling functions, but it is nowhere
> documented that this should be so and why.

ad_sample.c seems to have a comment saying "note: maxlen will be always
greater or equal to sh->audio_out_minsize". Maybe that's the original
intention, in practice some decoders rely on it being greater or equal
to minlen+audio_out_minsize...

> > I think allowing return of partial data with some channels missing is

> Huh? My suggestion wouldn't cause that to happen, it would still round
> correctly.

Yes, I misread what the suggestion was.

> But it would continue to work e.g. in the case that minlen > maxlen.
> Well, all those cases would be a bug I guess, but I prefer more
> resilient code if it's not too complicated.

Making the decoders more resilient to bad parameters (beyond avoiding
security vulnerabilities) also makes those bugs harder to notice and
find. And if not all decoders are equally careful, one decoder silently
working despite bugs can help those bugs stay and allow security
exploits against another decoder.

> > There already are decoders which rely on that assumption, and I think
> > they shouldn't all need extra checks. They're all called through
> > dec_audio.c, having the check there is better. In fact there already is
> > a check but it doesn't quite match what is actually required, I'll fix
> > it.
> 
> I don't disagree, but I'd like to point out that this is the usual case:
> If checks and the code assuming the checks are far apart, nobody in the
> end cares to check if the checks and the assumptions actually match...

The check I added to dec_audio.c had a comment noting that some decoders
rely on it, so hopefully nobody removes it without changing the
decoders.

If you add sanity checks for interface assumptions to the decoders I
think it's better to complain loudly or fail rather than silently work
around the problems when those assumptions turn out to be false.