1026854 - We need RAII for memory management in media/libcubeb/src/cubeb_resampler.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

See bug 1008079 comment 24. This should be a good bug for the Bugzilla starter.

Comment 1

[Clément Geiger [:cgg]]

Attachment: WIP

Comment 2

[Clément Geiger [:cgg]]

Attachment: Add an utilitary class for array RAII

:-*

Comment 3

[Clément Geiger [:cgg]]

Attachment: Add an utilitary class for array RAII

:3

Comment 4

[Clément Geiger [:cgg]]

Attachment: Add an utilitary class for array RAII

Voila, on y est.

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8443960 [details] [diff] [review]
Add an utilitary class for array RAII

Looks good.

Comment 6

[Paul Adenot (:padenot)]

https://tbpl.mozilla.org/?tree=Try&rev=105ba1428b2d

Comment 7

[Paul Adenot (:padenot)]

Thanks !

https://hg.mozilla.org/integration/mozilla-inbound/rev/619b13c76a5f

Comment 8

[Simon Lindholm]

> , leftover_frames_buffer(auto_array<uint8_t>(frames_to_bytes(params, leftover_frame_size)))
This only works because all compilers involved happen to do copy elision: http://en.wikipedia.org/wiki/Copy_elision

Maybe remove the "auto_array<uint8_t>(" and declare a private and undefined copy-constructor and assignment operator to avoid the footgun?

Comment 9

[Clément Geiger [:cgg]]

Indeed you're completely right. I'll do that now. Thank you!

Comment 10

[Clément Geiger [:cgg]]

Attachment: Bug 1026854 - Explicitly disable copy constructor for RAII array class. r=padenot

And voila!

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8443960 [details] [diff] [review]
Add an utilitary class for array RAII

Review of attachment 8443960 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +17,5 @@
> +template<typename T>
> +class auto_array
> +{
> +public:
> +  auto_array(uint32_t size)

Add 'explicit' to avoid confusing implicit conversion.

@@ +32,5 @@
> +    return data;
> +  }
> +
> +private:
> +  T * data;

|T * const data| for const-correctness.

@@ +127,4 @@
>    // A little buffer to store the leftover frames,
>    // that is, the samples not consumed by the resampler that we will end up
>    // using next time fill() is called.
> +  auto_array<uint8_t> leftover_frames_buffer;

const auto_array<> ...

@@ +131,2 @@
>    // A buffer to store frames that will be consumed by the resampler.
> +  auto_array<uint8_t> resampling_src_buffer;

const auto_array<> ...

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

We also need one for |SpeexResamplerState *|.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/619b13c76a5f

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

It would be a plus to have boundary check for the array class.

Comment 15

[Clément Geiger [:cgg]]

Hi JW, thanks for the feedback. I agree about the const. About the boundary check, I dit it at first and then realized that the [] operator was never used... so I took it out.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

I think it would be nice to add some mutator functions to replace memcpy at [1] for safe array operation since we can check array boundary in those mutators.

[1] http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/media/libcubeb/src/cubeb_resampler.cpp#l157

Comment 17

[Clément Geiger [:cgg]]

Attachment: 1026854

Comment 18

[Clément Geiger [:cgg]]

Comment on attachment 8447493 [details] [diff] [review]
1026854

Non.