[MSE] appendbuffer should return an error when invalid data is being added

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(8 attachments)

(Assignee)

Description

2 years ago
Per spec:
https://w3c.github.io/media-source/index.html#sourcebuffer-segment-parser-loop

"If the input buffer contains bytes that violate the SourceBuffer byte stream format specification, then run the append error algorithm and abort this algorithm.
"

However, we do not do that. other than for the init segment, we ignore all errors and skip every invalid bytes until we find a media segment that starts properly.

Additionally, the various MediaDataDemuxer never return an error, they simply demux as much as they can. In case of error they always returns EOS.
One question around that:
Should we try and find all possible errors in streams, including for things we may not actually need?

E.g., in bug 1314441, I intend to remove more of libstagefright, including the parsing of '----' boxes, which contain some information we are currently parsing but then not actually using!
https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#2365
(And I wouldn't be surprised if we were already skipping some "uninteresting" boxes.)

So with that we could accept streams that, strictly speaking, "violate the SourceBuffer byte stream format specification", however we would still be capable of parsing and (probably) playing them.

Is that a good or bad thing?
I can think of arguments going both ways, most importantly: Play whenever possible (good for the user) vs strict compliance (bad for Mozilla / the Web, because we could play things that other compliant browsers rightly would refuse).
(Assignee)

Updated

2 years ago
See Also: → bug 1315211
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → jyavenard

Comment 10

2 years ago
mozreview-review
Comment on attachment 8807989 [details]
Bug 1314533: [MSE] P1. Change member prototype.

https://reviewboard.mozilla.org/r/90902/#review90606

::: dom/media/mediasource/ContainerParser.cpp:185
(Diff revision 1)
> -    if (mLastMapping && (initSegment || IsMediaSegmentPresent(aData))) {
> +    if (mLastMapping && (initSegment ||
> +        NS_SUCCEEDED(IsMediaSegmentPresent(aData)))) {

Style: &&/|| alignments are confusing. I think you should just move '(initSegment ||' to the following line, together with 'NS_SUCCEEDED...'.
Attachment #8807989 - Flags: review?(gsquelart) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8807990 [details]
Bug 1314533: [MSE] P2. Reject appendBuffer if invalid data found.

https://reviewboard.mozilla.org/r/90904/#review90608
Attachment #8807990 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8807989 [details]
Bug 1314533: [MSE] P1. Change member prototype.

https://reviewboard.mozilla.org/r/90902/#review90606

> Style: &&/|| alignments are confusing. I think you should just move '(initSegment ||' to the following line, together with 'NS_SUCCEEDED...'.

tell that to clang-format!
That's disappointing of clang. :-(

Comment 14

2 years ago
mozreview-review
Comment on attachment 8807991 [details]
Bug 1314533: [MSE] P3. Reject invalid MP4 boxes.

https://reviewboard.mozilla.org/r/90906/#review90610

::: dom/media/mediasource/ContainerParser.cpp:394
(Diff revision 1)
> -        uint32_t type = reader.ReadU32();
> +        mp4_demuxer::AtomType type = mp4_demuxer::AtomType(reader.ReadU32());
>          MSE_DEBUGV(AtomParser ,"Checking atom:'%c%c%c%c' @ %u",
>                     typec[0], typec[1], typec[2], typec[3],
>                     (uint32_t)reader.Offset() - 8);
> +
> +        for (uint32_t i = 0; i < ArrayLength(validBoxes); i++) {

Suggestion: Range-for should work here, e.g., 'for (const mp4_demuxer::AtomType& validBox : validBoxes)'.

::: dom/media/mediasource/ContainerParser.cpp:402
(Diff revision 1)
> +          mLastInvalidBox[0] = typec[3];
> +          mLastInvalidBox[1] = typec[2];
> +          mLastInvalidBox[2] = typec[1];
> +          mLastInvalidBox[3] = typec[0];

This assumes a big-endian system (I think). Would you be able to get the correct order in a platform-independent manner?
(Though in the end it is not *that* critical, as it's only a comment inside an error, so I'm fine with getting this right in another bug.)

::: dom/media/mediasource/ContainerParser.cpp:410
(Diff revision 1)
> -            mp4_demuxer::AtomType(type) == initAtom) {
> +              mp4_demuxer::AtomType(type) == initAtom) {
> -          mInitOffset = Some(reader.Offset());
> +            mInitOffset = Some(reader.Offset());
> -        }
> +          }

Typo? Remove added indentation.
Attachment #8807991 - Flags: review?(gsquelart) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8807992 [details]
Bug 1314533: [MSE] P4. Reject invalid webm block.

https://reviewboard.mozilla.org/r/90908/#review90614
Attachment #8807992 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8807991 [details]
Bug 1314533: [MSE] P3. Reject invalid MP4 boxes.

https://reviewboard.mozilla.org/r/90906/#review90610

> This assumes a big-endian system (I think). Would you be able to get the correct order in a platform-independent manner?
> (Though in the end it is not *that* critical, as it's only a comment inside an error, so I'm fine with getting this right in another bug.)

box type *is* stored in big-endian. AFAIK, we only support little endian system.

Comment 17

2 years ago
mozreview-review
Comment on attachment 8807993 [details]
Bug 1314533: P5. Abort when webm parser encounter an error.

https://reviewboard.mozilla.org/r/90910/#review90628
Attachment #8807993 - Flags: review?(kinetik) → review+

Comment 18

2 years ago
mozreview-review
Comment on attachment 8807994 [details]
Bug 1314533: [MSE] P6. Allow to detect error during preliminary container parsing.

https://reviewboard.mozilla.org/r/90912/#review90624

r+ assuming the following questions don't raise any actual concerns.
In which case some extra comments in the code should help understand what will happen in the NOT_AVAILABLE case.

::: dom/media/mediasource/TrackBuffersManager.cpp:675
(Diff revision 1)
> +    if (!NS_SUCCEEDED(newData) && newData.Code() != NS_ERROR_NOT_AVAILABLE) {
> +      RejectAppend(newData, __func__);
> +      return;
> +    }
>      mProcessedInput += mInputBuffer->Length();

Can you claim to have processed the input in the NS_ERROR_NOT_AVAILABLE case?

Shouldn't you exit the parser loop with a NeedMoreData() in this case?

And will you restart processing this past data when the next buffer if appended?
Attachment #8807994 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8807994 [details]
Bug 1314533: [MSE] P6. Allow to detect error during preliminary container parsing.

https://reviewboard.mozilla.org/r/90912/#review90624

> Can you claim to have processed the input in the NS_ERROR_NOT_AVAILABLE case?
> 
> Shouldn't you exit the parser loop with a NeedMoreData() in this case?
> 
> And will you restart processing this past data when the next buffer if appended?

this is not what NS_ERROR_NOT_AVAILABLE means: it only indicates that the buffered parser hasn't found a new buffered range. This can happen with partial data, in which case the previous call would have already returned the full buffer range.
We only want to reject the data if an actual error was found.
We don't reject the appendBuffer if NS_ERROR_NOT_AVAILABLE is returned.

Comment 20

2 years ago
mozreview-review
Comment on attachment 8807996 [details]
Bug 1314533: [MSE] P8. Update wpt expected results.

https://reviewboard.mozilla.org/r/90916/#review90632
Attachment #8807996 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8807995 [details]
Bug 1314533: Do not expect init segment to be parsed when appending corrupted data.

https://reviewboard.mozilla.org/r/90914/#review90662
Attachment #8807995 - Flags: review?(gsquelart) → review+

Comment 32

2 years ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d85e1b19808
[MSE] P1. Change member prototype. r=gerald
https://hg.mozilla.org/integration/autoland/rev/33c8e33b0bb0
[MSE] P2. Reject appendBuffer if invalid data found. r=gerald
https://hg.mozilla.org/integration/autoland/rev/754c160ecbed
[MSE] P3. Reject invalid MP4 boxes. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7c84e5453411
[MSE] P4. Reject invalid webm block. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ab4d4dfee074
P5. Abort when webm parser encounter an error. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/ca6e6783a379
[MSE] P6. Allow to detect error during preliminary container parsing. r=gerald
https://hg.mozilla.org/integration/autoland/rev/87882fa5ef49
Do not expect init segment to be parsed when appending corrupted data. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f26d5e27e1b6
[MSE] P8. Update wpt expected results. r=gerald
(Assignee)

Updated

2 years ago
Depends on: 1322587

Updated

2 years ago
Depends on: 1329480
You need to log in before you can comment on or make changes to this bug.