Fix memory corruption #25

gavv · 2019-11-12T13:38:56Z

Hi and thanks for sharing this library.

Currently, all decode methods have two problems:

They pass to opus_decode() the total number of samples in output buffer. But according to documentation, they should pass number of samples per channel. In result, in two-channel mode opus will think that the buffer is twice larger than it is. This may lead to memory corruption which is actually reproducing in my application. It's not reproduced each time. It seems that opus usually determines the frame size from its contents and doesn't touch the memory beyond that size.
They use cap() instead of len(). I believe this is a bad decision for API because if I pass data[:n] to a function, I expect that the memory beyond n will not be touched. But with current API it may happen if the slice capacity is larger than the slice length. Such bugs may be very hard to find, especially if your slice comes from some other component and refers to a part of a larger memory region (e.g. in pool).

This patch fixes both issues. If you don't agree with the second point, which may be arguable, I can revert the second part of the fix and use cap() instead of len().

I've also added a test. This test fails before applying the patch and succeeds after applying it.

hraban · 2019-11-12T13:51:03Z

Ugh. How on earth is this possible. I spent a million years looking at these docs and here we are.

Thanks for the patch. I'll have a look over tonight, but it looks like you're right.

Please also add yourself to the AUTHORS list.

gavv · 2019-11-12T13:55:03Z

Please also add yourself to the AUTHORS list.

Done.

ydnar · 2019-11-13T22:44:36Z

Yikes. Thanks @gavv!

hraban · 2019-11-14T15:24:07Z

Hi gavv I don't have time to look at the cap vs len thing for at least another week. I like the idea but I need to think through the possible implications for people already depending on this code.

In the mean time, especially now that this bug is out in the open, I'd like to merge the memory issue. Would you mean removing the len/cap code from this PR and creating another one?

Thanks!

hraban

thanks! 👍

hraban · 2019-11-14T15:24:38Z

decoder.go

@@ -74,12 +76,15 @@ func (dec *Decoder) Decode(data []byte, pcm []int16) (int, error) {
 	if len(pcm) == 0 {
 		return 0, fmt.Errorf("opus: target buffer empty")
 	}
+	if len(pcm)%dec.channels != 0 {
+		return 0, fmt.Errorf("opus: output buffer length must be multiple of channels")


Suggested change

return 0, fmt.Errorf("opus: output buffer length must be multiple of channels")

return 0, fmt.Errorf("opus: target buffer length must be multiple of channels")

hraban · 2019-11-14T15:24:56Z

decoder.go

@@ -99,12 +104,15 @@ func (dec *Decoder) DecodeFloat32(data []byte, pcm []float32) (int, error) {
 	if len(pcm) == 0 {
 		return 0, fmt.Errorf("opus: target buffer empty")
 	}
+	if len(pcm)%dec.channels != 0 {
+		return 0, fmt.Errorf("opus: output buffer length must be multiple of channels")


Suggested change

return 0, fmt.Errorf("opus: output buffer length must be multiple of channels")

return 0, fmt.Errorf("opus: target buffer length must be multiple of channels")

hraban · 2019-11-14T15:25:08Z

decoder.go

@@ -125,13 +133,15 @@ func (dec *Decoder) DecodeFEC(data []byte, pcm []int16) error {
 	if len(pcm) == 0 {
 		return fmt.Errorf("opus: target buffer empty")
 	}
-
+	if len(pcm)%dec.channels != 0 {
+		return fmt.Errorf("opus: output buffer length must be multiple of channels")


Suggested change

return fmt.Errorf("opus: output buffer length must be multiple of channels")

return fmt.Errorf("opus: target buffer length must be multiple of channels")

hraban · 2019-11-14T15:25:17Z

decoder.go

@@ -152,12 +162,15 @@ func (dec *Decoder) DecodeFECFloat32(data []byte, pcm []float32) error {
 	if len(pcm) == 0 {
 		return fmt.Errorf("opus: target buffer empty")
 	}
+	if len(pcm)%dec.channels != 0 {
+		return fmt.Errorf("opus: output buffer length must be multiple of channels")


Suggested change

return fmt.Errorf("opus: output buffer length must be multiple of channels")

return fmt.Errorf("opus: target buffer length must be multiple of channels")

hraban · 2019-11-14T15:25:59Z

opus_test.go

+	t.Run("larger-buffer-float32-fec", func(t *testing.T) {
+		decodeFecFloat32(t, encodeFrame(t), FRAME_SIZE+1, false)
+	})
+}


*sigh, go and generics...

hraban · 2019-11-14T15:28:28Z

decoder.go

 	n := int(C.opus_decode(
 		dec.p,
 		(*C.uchar)(&data[0]),
 		C.opus_int32(len(data)),
 		(*C.opus_int16)(&pcm[0]),
-		C.int(cap(pcm)),
+		C.int(len(pcm)/dec.channels),


Suggested change

C.int(len(pcm)/dec.channels),

C.int(cap(pcm)/dec.channels),

sticking to cap for this PR, will think on cap/len later.

hraban · 2019-11-14T15:28:40Z

decoder.go

 	n := int(C.opus_decode_float(
 		dec.p,
 		(*C.uchar)(&data[0]),
 		C.opus_int32(len(data)),
 		(*C.float)(&pcm[0]),
-		C.int(cap(pcm)),
+		C.int(len(pcm)/dec.channels),


Suggested change

C.int(len(pcm)/dec.channels),

C.int(cap(pcm)/dec.channels),

hraban · 2019-11-14T15:28:50Z

decoder.go

 	n := int(C.opus_decode(
 		dec.p,
 		(*C.uchar)(&data[0]),
 		C.opus_int32(len(data)),
 		(*C.opus_int16)(&pcm[0]),
-		C.int(cap(pcm)),
+		C.int(len(pcm)/dec.channels),


Suggested change

C.int(len(pcm)/dec.channels),

C.int(cap(pcm)/dec.channels),

hraban · 2019-11-14T15:29:02Z

decoder.go

 	n := int(C.opus_decode_float(
 		dec.p,
 		(*C.uchar)(&data[0]),
 		C.opus_int32(len(data)),
 		(*C.float)(&pcm[0]),
-		C.int(cap(pcm)),
+		C.int(len(pcm)/dec.channels),


Suggested change

C.int(len(pcm)/dec.channels),

C.int(cap(pcm)/dec.channels),

hraban · 2019-11-14T15:37:44Z

Forget about the github UI, I've done it old school locally and just pushed it. ~~Your patch has been included in v2~~: cb9658a -- not included just yet, can't make tests work locally. out of time again so will have to look at this... at some point in my life.

gavv · 2019-11-14T16:59:14Z

I like the idea but I need to think through the possible implications for people already depending on this code.

I'd suggest you to make a v3 release. This patch, even with cap() instead of len(), breaks the API anyway.

Would you mean removing the len/cap code from this PR and creating another one?

Sure.

gavv · 2019-11-14T16:59:42Z

I've revered cap/len change, applied your suggestions, and fixed tests.

hraban · 2019-11-14T17:04:25Z

I will have to backport this to v2 and v1 because it's a security issue (for future reference, by the way: if you find memory corruption bug, I recommend reporting it in private at least initially, because putting it out in the open significantly increases the pressure to fix it asap :/ it's not ideal timing at the mo 😭).

hraban · 2019-11-14T17:57:53Z

I'd suggest you to make a v3 release. This patch, even with cap() instead of len(), breaks the API anyway.

Isn't the error in our invocation of libopus? as far as I can see, fixing this should work backwards compatibly for everyone. I was calling libopus wrong, but users of this lib just pass in an array with a cap, no further updates needed (cue also the other tests which still work fine).

Any specific reason you were thinking of a v3?

gavv · 2019-11-14T18:22:08Z

You're right. I was thinking about the behavior change. There are three cases:

user passes a buffer smaller than the actual frame
user passes a buffer of the exact same size
user passes a larger buffer

Old behavior:

corrupted memory 2. usually correct 3. usually correct

New behavior:

error 2. correct 3. correct

So we're changing a (probably silent) memory corruption to an error. I guess this should be considered as a compatible change :)

gavv · 2019-11-17T07:38:17Z

I'd like to push more commits into v2 branch of my fork, which are not related to this PR. Github does not allow me to switch PR's branch, so I'm reopening it with another branch: #26.

I've also replaced len() with cap() in new buffer size checks. I didn't touch the old checks.

gavv force-pushed the v2 branch from 21c174a to 659bb09 Compare November 12, 2019 13:54

hraban approved these changes Nov 14, 2019

View reviewed changes

hraban reviewed Nov 14, 2019

View reviewed changes

Fix memory corruption

31c4366

gavv force-pushed the v2 branch from 659bb09 to 31c4366 Compare November 14, 2019 16:57

gavv closed this Nov 17, 2019

gavv mentioned this pull request Nov 17, 2019

Fix memory corruption #26

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory corruption #25

Fix memory corruption #25

gavv commented Nov 12, 2019 •

edited

Loading

hraban commented Nov 12, 2019

gavv commented Nov 12, 2019

ydnar commented Nov 13, 2019

hraban commented Nov 14, 2019

hraban left a comment

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban Nov 14, 2019

hraban commented Nov 14, 2019 •

edited

Loading

gavv commented Nov 14, 2019

gavv commented Nov 14, 2019

hraban commented Nov 14, 2019

hraban commented Nov 14, 2019

gavv commented Nov 14, 2019

gavv commented Nov 17, 2019

	return 0, fmt.Errorf("opus: output buffer length must be multiple of channels")
	return 0, fmt.Errorf("opus: target buffer length must be multiple of channels")

	return fmt.Errorf("opus: output buffer length must be multiple of channels")
	return fmt.Errorf("opus: target buffer length must be multiple of channels")

Fix memory corruption #25

Fix memory corruption #25

Conversation

gavv commented Nov 12, 2019 • edited Loading

hraban commented Nov 12, 2019

gavv commented Nov 12, 2019

ydnar commented Nov 13, 2019

hraban commented Nov 14, 2019

hraban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hraban commented Nov 14, 2019 • edited Loading

gavv commented Nov 14, 2019

gavv commented Nov 14, 2019

hraban commented Nov 14, 2019

hraban commented Nov 14, 2019

gavv commented Nov 14, 2019

gavv commented Nov 17, 2019

gavv commented Nov 12, 2019 •

edited

Loading

hraban commented Nov 14, 2019 •

edited

Loading