Closed Bug 826171 Opened 7 years ago Closed 7 years ago

strndup implementation in memory/build/mozmemory_wrap.c is broken

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- fixed
firefox-esr10 --- wontfix
firefox-esr17 --- wontfix
b2g18 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

That strndup definition, inherited from bug 559263, doesn't handle a partial copy of another string properly.
Summary: strndup implementation in memory/build/mozmemory_wrap.h is broken → strndup implementation in memory/build/mozmemory_wrap.c is broken
Attachment #697358 - Flags: review?(justin.lebar+bug)
Comment on attachment 697358 [details] [diff] [review]
Fix strndup in memory/build/mozmemory_wrap.c

o.O
Attachment #697358 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/mozilla-central/rev/facbbe435906
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
This sounds like the kind of bug that should get fixed everywhere. Should we request approval on the patch for most branches?
For reference, this affects modules loaded into a firefox process, which expect the libc implementation:

https://bugzilla.redhat.com/show_bug.cgi?id=922904
(In reply to Kai Engert (:kaie) from comment #6)
> This sounds like the kind of bug that should get fixed everywhere. Should we
> request approval on the patch for most branches?

I think we didn't take it on branches because we had no reason to believe that this fix had any effect on users, and that's a criterion that's usually applied to uplift requests.

If that belief is not true, I'd certainly be OK uplifting this to branches.
(In reply to Justin Lebar [:jlebar] from comment #8)
> (In reply to Kai Engert (:kaie) from comment #6)
> > This sounds like the kind of bug that should get fixed everywhere. Should we
> > request approval on the patch for most branches?
> 
> I think we didn't take it on branches because we had no reason to believe
> that this fix had any effect on users, and that's a criterion that's usually
> applied to uplift requests.
> 
> If that belief is not true, I'd certainly be OK uplifting this to branches.

Firefox exports these functions into the global symbol namespace. I'm not sure what the reasoning is for doing that. 

Because these functions override glibc functions, bugs in them have effects on any module loaded by firefox (imagine NPAPI plugin, PKCS#11 module, or binary add-on) making use of the given function. In this case the bug was seen when using the p11-kit-trust.so PKCS#11 module:

https://bugzilla.redhat.com/show_bug.cgi?id=922904
Comment on attachment 697358 [details] [diff] [review]
Fix strndup in memory/build/mozmemory_wrap.c

Proposing to fix on all active branches, in order to prevent random misbehaviour.
Attachment #697358 - Flags: approval-mozilla-esr17?
Attachment #697358 - Flags: approval-mozilla-beta?
Attachment #697358 - Flags: approval-mozilla-aurora?
Target Milestone: mozilla20 → mozilla22
It's already in 20.0 Beta and 21.0a2.
Target Milestone: mozilla22 → mozilla20
Comment on attachment 697358 [details] [diff] [review]
Fix strndup in memory/build/mozmemory_wrap.c

It's already on aurora, beta, and doesn't affect esr17 on linux, I'm pretty sure it doesn't affect esr17 on mac, and it probably doesn't affect esr17 on windows.
Attachment #697358 - Flags: approval-mozilla-esr17?
Attachment #697358 - Flags: approval-mozilla-beta?
Attachment #697358 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.