Message ID | c703c21c2cdd0cd0f294466f7bde88329df2851b.1570398124.git.barsnick@gmx.net |
---|---|
State | New |
Headers | show |
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 [...]
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--");
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