1143096 - WebMBufferedParser ctor fails to initialize all fields

Status: RESOLVED FIXED

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

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Gerald - can you see if this is still an issue?

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: MozReview Request: Bug 1143096 - Init all WebMBufferedParser members - r?jya

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/

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a20442b6bdaa

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8715069 [details]
MozReview Request: Bug 1143096 - Init all WebMBufferedParser members - r?jya

Prefer for Mathew to review this.

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8715069 [details]
MozReview Request: Bug 1143096 - Init all WebMBufferedParser members - r?jya

https://reviewboard.mozilla.org/r/33323/#review30071

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/177446e7e45d

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/177446e7e45d