1179550 - Potential OOB reads due to use of strncpy in StackWalk.cpp

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Eric Rahm [:erahm]]

The strings copied in MozDescribeCodeAddress and Demangle w/ strncpy [1][2][3] are potentially not null terminated. This can lead to future OOB reads.

This appears to be a regression from bug 1172216 where |PL_strncpyz| was swapped back out for |strncpy| (most likely b/c mozglue can't use NSPR).

[1] https://hg.mozilla.org/mozilla-central/annotate/ca0fd580a9ce/mozglue/misc/StackWalk.cpp#l770
[2] https://hg.mozilla.org/mozilla-central/annotate/ca0fd580a9ce/mozglue/misc/StackWalk.cpp#l1037
[3] https://hg.mozilla.org/mozilla-central/annotate/ca0fd580a9ce/mozglue/misc/StackWalk.cpp#l852

Comment 1

[Benoit Girard (:BenWa)]

Spoke with :erahm. The difference between |PL_strncpyz| and |strncpy| is that the former has a stronger guarantee for the return value:

82  * [...] The destination string is always terminated with a '\0',
83  * unlike the traditional libc implementation.

We should be safe if we do an unconditional:
buff[len - 1] = '\0'

Eric warns that |PL_strncpyz| is faster:
erahm: it's also more efficient on bsd-ish systems where strncpy will zero out the whole buffer
erahm: I profiled at one point and it was an issue, I think either deadlock detector or DMD was passing in a 4k buffer, it was ridiculous

We wont optimize prematurely for now but if it shows up here in profiles later we will fix it.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=935f55fae0b5

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8629062 [details] [diff] [review]
patch

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

r=me

Comment 4

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01e460df7b65

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/01e460df7b65

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Is this worth taking on 41?

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 8629062 [details] [diff] [review]
patch

I think we should, it's low risk. But also I think there's just debugging code that triggers this code so it's not high impact either.

Approval Request Comment
[Feature/regressing bug #]: bug 1172216
[User impact if declined]: Probably none, the stackwalking is only used during debugging features
[Describe test coverage new/current, TreeHerder]: Has been on central for awhile now.
[Risks and why]: Fairly low, we're forcing the buffers to be null terminated.
[String/UUID change made/needed]: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8629062 [details] [diff] [review]
patch

Fx41 is on Beta now.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8629062 [details] [diff] [review]
patch

This fix has been in Nightly and Aurora for over 2 months. Seems safe to uplift to Beta41.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/abe7c2958613