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

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 on attachment 8629062 [details] [diff] [review] patch Review of attachment 8629062 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Is this worth taking on 41?

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 on attachment 8629062 [details] [diff] [review] patch Fx41 is on Beta now.

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.