[FFmpeg-devel,v3,2/3] avformat/mpjpegdec: fix strict boundary search string

According to RFC1341, the multipart boundary indicated by the
Content-Type header must be prepended by CRLF + "--", and followed
by CRLF. In the case of strict MIME header boundary handling, the
"--" was forgotten to add.

Fixes trac #7921.

A side effect is that this coincidentally breaks enforcement of
strict MIME headers against servers running motion < 3.4.1, where
the boundary announcement in the HTTP headers incorrectly used the
prefix "--", which exactly matched this bug's behavior.

Signed-off-by: Moritz Barsnick <barsnick@gmx.net>
---
 libavformat/mpjpegdec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--
2.20.1

lgtm

On 10/7/19, Moritz Barsnick <barsnick@gmx.net> wrote:
> According to RFC1341, the multipart boundary indicated by the
> Content-Type header must be prepended by CRLF + "--", and followed
> by CRLF. In the case of strict MIME header boundary handling, the
> "--" was forgotten to add.
>
> Fixes trac #7921.
>
> A side effect is that this coincidentally breaks enforcement of
> strict MIME headers against servers running motion < 3.4.1, where
> the boundary announcement in the HTTP headers incorrectly used the
> prefix "--", which exactly matched this bug's behavior.
>
> Signed-off-by: Moritz Barsnick <barsnick@gmx.net>
> ---
>  libavformat/mpjpegdec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c
> index c0ffaf616e..24bf232db2 100644
> --- a/libavformat/mpjpegdec.c
> +++ b/libavformat/mpjpegdec.c
> @@ -302,8 +302,9 @@ static int mpjpeg_read_packet(AVFormatContext *s,
> AVPacket *pkt)
>              boundary = mpjpeg_get_boundary(s->pb);
>          }
>          if (boundary != NULL) {
> -            mpjpeg->boundary = boundary;
> -            mpjpeg->searchstr = av_asprintf( "\r\n%s\r\n", boundary );
> +            mpjpeg->boundary = av_asprintf("--%s", boundary);
> +            mpjpeg->searchstr = av_asprintf("\r\n--%s\r\n", boundary);
> +            av_freep(&boundary);
>          } else {
>              mpjpeg->boundary = av_strdup("--");
>              mpjpeg->searchstr = av_strdup("\r\n--");
> --
> 2.20.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On Mon, Oct 07, 2019 at 12:19:29AM +0200, Moritz Barsnick wrote:
> According to RFC1341, the multipart boundary indicated by the
> Content-Type header must be prepended by CRLF + "--", and followed
> by CRLF. In the case of strict MIME header boundary handling, the
> "--" was forgotten to add.
> 
> Fixes trac #7921.
> 

> A side effect is that this coincidentally breaks enforcement of
> strict MIME headers against servers running motion < 3.4.1, where
> the boundary announcement in the HTTP headers incorrectly used the
> prefix "--", which exactly matched this bug's behavior.

Can this be worked around, by for example detecting that case and
supporting it ?
Or maybe i misunderstand 

Thanks

[...]

On Tue, Oct 08, 2019 at 11:32:09 +0200, Michael Niedermayer wrote:
> > A side effect is that this coincidentally breaks enforcement of
> > strict MIME headers against servers running motion < 3.4.1, where
> > the boundary announcement in the HTTP headers incorrectly used the
> > prefix "--", which exactly matched this bug's behavior.
>
> Can this be worked around, by for example detecting that case and
> supporting it ?
> Or maybe i misunderstand

It could, but should we really?
- Strict MIME enforcement never really worked upto now, due to a
  programming bug (possibly a misunderstanding of the spec) in ffmpeg.
  This makes me think that hardly anyone ever even used
  -strict_mime_boundary.
- It would now, after this patch, happen to fail with a server which
  implements the same misinterpretation of the spec.

If you want to support both interpretations, you'd need to search for
two different boundaries now. Quite some bloat, if you ask me. I don't
know if it's worth the effort, i.e. whether this will even hit anyone,
as "-strict_mime_boundary" is off by default.

I vote against, but I probably don't decide.

Cheers,
Moritz

--- Long version: ---
To be more clear:
- The HTTP server announces
  Content-type: multipart/x-mixed-replace; boundary="AnyArbitraryBoundaryString"
  It is supposed to use a boundary "\r\n--AnyArbitraryBoundaryString\r\n" in
  the content stream.
- In ffmpeg:
  - we search for a boundary "\r\n--" in standard mode.
  - we search for a boundary "\r\n--AnyArbitraryBoundaryString\r\n" in strict mode.
  - we used to search for a boundary "\r\nAnyArbitraryBoundaryString\r\n" in strict mode (before this fix).

motion < 3.4.1 actually announes:
  Content-type: multipart/x-mixed-replace; boundary="--BoundaryString"
and uses the content separator:
  --BoundaryString
(incorrectly).

This has always worked with ffmpeg's non-strict mode.
This used to work in ffmpeg's strict MIME mode (but incorrectly so).
This no longer works with the fixed ffmpeg strict MIME mode.

Possible change:
ffmpeg parses 'Content-type: multipart/x-mixed-replace; boundary=' as
before.

If the parsed announced Boundary begins with "--", i.e. "--ABCXYZ",
then ffmpeg would check for boundaries:
"--ABCXYZ" and "----ABCXYZ" (the latter being the strictly correct
one).

If the parsed announced Boundary does not begin with "--", i.e. "ABCXYZ",
then ffmpeg would check for boundary:
"--ABCXYZ"

On Tue, Oct 08, 2019 at 01:55:27PM +0200, Moritz Barsnick wrote:
> On Tue, Oct 08, 2019 at 11:32:09 +0200, Michael Niedermayer wrote:
> > > A side effect is that this coincidentally breaks enforcement of
> > > strict MIME headers against servers running motion < 3.4.1, where
> > > the boundary announcement in the HTTP headers incorrectly used the
> > > prefix "--", which exactly matched this bug's behavior.
> >
> > Can this be worked around, by for example detecting that case and
> > supporting it ?
> > Or maybe i misunderstand
> 
> It could, but should we really?
> - Strict MIME enforcement never really worked upto now, due to a
>   programming bug (possibly a misunderstanding of the spec) in ffmpeg.
>   This makes me think that hardly anyone ever even used
>   -strict_mime_boundary.
> - It would now, after this patch, happen to fail with a server which
>   implements the same misinterpretation of the spec.
> 

> If you want to support both interpretations, you'd need to search for
> two different boundaries now. Quite some bloat, if you ask me. I don't
> know if it's worth the effort, i.e. whether this will even hit anyone,
> as "-strict_mime_boundary" is off by default.

I hoped that this server version can easily be detected so only one
search is needed


> 
> I vote against, but I probably don't decide.

I abstain from voting, was more a thought that more stuff working out
of the box wouldnt be bad 

thx


[...]

On Wed, Oct 09, 2019 at 16:34:37 +0200, Michael Niedermayer wrote:
> > If you want to support both interpretations, you'd need to search for
> > two different boundaries now. Quite some bloat, if you ask me. I don't
> > know if it's worth the effort, i.e. whether this will even hit anyone,
> > as "-strict_mime_boundary" is off by default.
>
> I hoped that this server version can easily be detected so only one
> search is needed

Sure, it announces itself in the reply headers with
Server: Motion/X.Y.Z

I'm not sure ffmpeg evaluates all headers yet. The boundary is fetched
from the headers of the URL with

av_opt_get(pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type);

Otherwise we'd need the heuristics named in the previous mail.

> > I vote against, but I probably don't decide.
>
> I abstain from voting, was more a thought that more stuff working out
> of the box wouldnt be bad

Who decides, and who pushes? Can we perhaps get this patch set through
and fix this up later?

Gruß,
Moritz

On Thu, Oct 10, 2019 at 11:28:47AM +0200, Moritz Barsnick wrote:
> On Wed, Oct 09, 2019 at 16:34:37 +0200, Michael Niedermayer wrote:
> > > If you want to support both interpretations, you'd need to search for
> > > two different boundaries now. Quite some bloat, if you ask me. I don't
> > > know if it's worth the effort, i.e. whether this will even hit anyone,
> > > as "-strict_mime_boundary" is off by default.
> >
> > I hoped that this server version can easily be detected so only one
> > search is needed
> 
> Sure, it announces itself in the reply headers with
> Server: Motion/X.Y.Z
> 
> I'm not sure ffmpeg evaluates all headers yet. The boundary is fetched
> from the headers of the URL with
> 
> av_opt_get(pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type);
> 
> Otherwise we'd need the heuristics named in the previous mail.
> 
> > > I vote against, but I probably don't decide.
> >
> > I abstain from voting, was more a thought that more stuff working out
> > of the box wouldnt be bad
> 
> Who decides, and who pushes? Can we perhaps get this patch set through
> and fix this up later?

ok, will apply

thx

[...]