Closed Bug 1143096 Opened 5 years ago Closed 4 years ago

WebMBufferedParser ctor fails to initialize all fields

Categories

(Core :: Audio/Video: Playback, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: erahm, Assigned: gerald)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 749810][CID 1286102][CID 1286260][CID 1286510])

Attachments

(1 file)

Coverity indicates that |WebMBufferedParser| does not initialize all fields [1]. In particular this leads to a situation where it might be possible that |mSkipBytes| is used uninitialized in |WebMBufferedParser::Append| [2].

>   3. uninit_member: Non-static class member mNextState is not initialized in this constructor nor in any functions that it calls.
>   5. uninit_member: Non-static class member mVIntLeft is not initialized in this constructor nor in any functions that it calls.
>   7. uninit_member: Non-static class member mBlockSize is not initialized in this constructor nor in any functions that it calls.
>   9. uninit_member: Non-static class member mClusterTimecode is not initialized in this constructor nor in any functions that it calls.
>   11. uninit_member: Non-static class member mClusterOffset is not initialized in this constructor nor in any functions that it calls.
>   13. uninit_member: Non-static class member mBlockOffset is not initialized in this constructor nor in any functions that it calls.
>   15. uninit_member: Non-static class member mBlockTimecode is not initialized in this constructor nor in any functions that it calls.
>   17. uninit_member: Non-static class member mBlockTimecodeLength is not initialized in this constructor nor in any functions that it calls.
>   19. uninit_member: Non-static class member mSkipBytes is not initialized in this constructor nor in any functions that it calls.

[1] https://hg.mozilla.org/mozilla-central/annotate/11506aaf7064/dom/media/webm/WebMBufferedParser.h#l52
[2] https://hg.mozilla.org/mozilla-central/annotate/11506aaf7064/dom/media/webm/WebMBufferedParser.cpp#l191
Whiteboard: [CID 749810][CID 1286102] → [CID 749810][CID 1286102][CID 1286260]
Whiteboard: [CID 749810][CID 1286102][CID 1286260] → [CID 749810][CID 1286102][CID 1286260][CID 1286510]
Assignee: nobody → jyavenard
Component: Audio/Video → Audio/Video: Playback
Gerald - can you see if this is still an issue?
Flags: needinfo?(gsquelart)
TL;DR: All ok except mClusterTimecode and mClusterOffset.


(In reply to Eric Rahm [:erahm] from comment #0)
> Coverity indicates that |WebMBufferedParser| does not initialize all fields.

That is still true today.

> In particular this leads to a situation where it might be possible that
> |mSkipBytes| is used uninitialized in |WebMBufferedParser::Append|.

mSkipBytes is:
Used in SKIP_DATA; Set in *all* preceding states PARSE_ELEMENT/default, READ_BLOCK_TIMECODE.
Used in CHECK_INIT_FOUND; Set in *only* preceding state PARSE_ELEMENT/TRACKS_ID.
So I don't think there is any uninitialized use of mSkipBytes.

Looking at the other ones:

> >   3. uninit_member: Non-static class member mNextState is not initialized in this constructor nor in any functions that it calls.
Used in READ_VINT_REST; Set in all preceding states PARSE_ELEMENT/TIMECODE_ID, PARSE_ELEMENT/TIMECODESCALE_ID, READ_VINT.
Used in SKIP_DATA; Set in all preceding states PARSE_ELEMENT/default, READ_BLOCK_TIMECODE.

> >   5. uninit_member: Non-static class member mVIntLeft is not initialized in this constructor nor in any functions that it calls.
Used in READ_VINT_REST; Set in all preceding states PARSE_ELEMENT/TIMECODE_ID, PARSE_ELEMENT/TIMECODESCALE_ID, READ_VINT.

> >   7. uninit_member: Non-static class member mBlockSize is not initialized in this constructor nor in any functions that it calls.
> >   13. uninit_member: Non-static class member mBlockOffset is not initialized in this constructor nor in any functions that it calls.
> >   15. uninit_member: Non-static class member mBlockTimecode is not initialized in this constructor nor in any functions that it calls.
> >   17. uninit_member: Non-static class member mBlockTimecodeLength is not initialized in this constructor nor in any functions that it calls.
Used in READ_BLOCK_TIMECODE; Set in only preceding state BLOCK_ID.

> >   9. uninit_member: Non-static class member mClusterTimecode is not initialized in this constructor nor in any functions that it calls.
Used in READ_BLOCK_TIMECODE (preceding state: PARSE_ELEMENT/BLOCK_ID)
Set in *unrelated* READ_CLUSTER_TIMECODE (preceding state: PARSE_ELEMENT/TIMECODE_ID)
This one looks like a real issue, it assumes that a TIMECODE_ID must precede any BLOCK_ID.

> >   11. uninit_member: Non-static class member mClusterOffset is not initialized in this constructor nor in any functions that it calls.
Used in READ_BLOCK_TIMECODE; Set in only preceding state PARSE_ELEMENT_ID/CLUSTER_ID.
Used in method EndSegmentOffset(), but only if mLastInitStartOffset was set to as well, which happens in PARSE_ELEMENT_ID/EBML_ID. So there could be a problem if EndSegmentOffset() is called when there is no EBML_ID and no CLUSTER_ID.


I'd be happy to follow up with a more detailed analysis, and see if a POC file that would highlight these issues could be created.
Flags: needinfo?(gsquelart)
Initialize all WebMBufferedParser members, mainly to remove compiler warnings.
'mClusterTimecode' and 'mClusterOffset' are probably genuine potential issues,
see bug 1143096 comment 2 for details.

Review commit: https://reviewboard.mozilla.org/r/33323/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33323/
Attachment #8715069 - Flags: review?(jyavenard)
Comment on attachment 8715069 [details]
MozReview Request: Bug 1143096 - Init all WebMBufferedParser members - r?jya

Prefer for Mathew to review this.
Attachment #8715069 - Flags: review?(jyavenard) → review?(kinetik)
Comment on attachment 8715069 [details]
MozReview Request: Bug 1143096 - Init all WebMBufferedParser members - r?jya

https://reviewboard.mozilla.org/r/33323/#review30071
Attachment #8715069 - Flags: review?(kinetik) → review+
https://hg.mozilla.org/mozilla-central/rev/177446e7e45d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.