1298275 - FlacFrameParser's AutoByteReader has a virtual destructor when its superclass doesn't

Status: RESOLVED FIXED

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

Description

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

ByteReader's destructor is not virtual.
AutoByteReader inherits from ByteReader, but declares its destructor virtual.

AutoByteReader is only used as a stack object in FlacFrameParser.cpp, so it's not a big problem at the moment, just a bit wasteful.

But if it was exposed to a wider audience (e.g., after bug 1298271), this could be ripe for abuse, e.g., if an AutoByteReader was stored in a pointer-to-ByteReader, the wrong destructor would be called.

So either we can make ~ByteReader virtual too, or make both non-virtual if we can ensure AutoByteReader won't get stored into a ByteReader*.

Comment 1

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

Attachment: Bug 1298275 - Make ~AutoByteReader non-virtual -

Also ByteReader and AutoByteReader are marked RAII, to help prevent misuses.

Review commit: https://reviewboard.mozilla.org/r/74480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74480/

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785183 [details]
Bug 1298275 - Make ~AutoByteReader non-virtual -

https://reviewboard.mozilla.org/r/74480/#review72344

::: media/libstagefright/binding/include/mp4_demuxer/ByteReader.h:19
(Diff revision 1)
>  
> +MOZ_RAII
>  class ByteReader
>  {
>  public:
>    ByteReader() : mPtr(nullptr), mRemaining(0) {}

Wouldn't it be easier to mark ~ByteReader as virtual?

Comment 3

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

Comment on attachment 8785183 [details]
Bug 1298275 - Make ~AutoByteReader non-virtual -

https://reviewboard.mozilla.org/r/74480/#review72344

Thank you for the quick review.

> Wouldn't it be easier to mark ~ByteReader as virtual?

But it would be so inefficient!
Also ByteReader points at an external buffer, so enforcing it to be an on-stack object prevents it being stored somewhere where it could outlive the external buffer.

Comment 4

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

Comment on attachment 8785183 [details]
Bug 1298275 - Make ~AutoByteReader non-virtual -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74480/diff/1-2/

Comment 5

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

Comment on attachment 8785183 [details]
Bug 1298275 - Make ~AutoByteReader non-virtual -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74480/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9b5e8c3cd84
Make ~AutoByteReader non-virtual - r=jya

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9b5e8c3cd84