Closed Bug 1241901 Opened 4 years ago Closed 4 years ago

nsAutoPtr should not be used to hold array

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

nsAutoPtr uses 'delete' to release the its owning memory instead of 'delete[]', which means if the target is an dynamically allocated array, using nsAutoPtr causes undefined behavior.

However, with compilers we are currently using, it doesn't cause anything catastrophic if it is an array of primitive values. (With non-primitives, it would crash in runtime.) And consequently, it is used in some places.

It should be replaced with nsAutoArrayPtr, or UniquePtr.

Some of the places which are using this:
https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp#162-163
https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/ipc/chromium/src/base/message_pump_libevent.h#195,211
https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/xpcom/tests/gtest/TestCloneInputStream.cpp#148
https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/dom/media/webspeech/synth/pico/nsPicoService.h#85
https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/widget/windows/nsWindowGfx.cpp#72

Probably we should just add a condition to nsAutoPtr that T must not be a primitive type. I don't think we would use it to hold a single primitive value.
It is also a defect detected by Coverity:
** CID 1349785:    (DELETE_ARRAY)
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 197 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 197 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 207 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 227 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 248 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 227 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 248 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 201 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 201 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 219 in nsNotifyAddrListener::OnNetlinkMessage(int)()
/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp: 211 in nsNotifyAddrListener::OnNetlinkMessage(int)()
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #0)
> [nsAutoPtr of arrays] should be replaced with nsAutoArrayPtr, or UniquePtr.

FYI: Given bug 1241518 comment 0 ("We'd like to remove nsAutoPtr entirely from the tree..."), I think you may want to replace nsAutoPtr with UniquePtr only (not nsAutoArrayPtr, which I'm guessing is as undesirable as nsAutoPtr).

Or even wait for bug 1241518 to be completed (with hint from this bug about using the correct array type in some places), as it might take care of your problem automatically!(?)
See Also: → 1241518
That's good news. I'll leave some messages there.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #0)
> nsAutoPtr uses 'delete' to release the its owning memory instead of
> 'delete[]', which means if the target is an dynamically allocated array,
> using nsAutoPtr causes undefined behavior.
> 
> However, with compilers we are currently using, it doesn't cause anything
> catastrophic if it is an array of primitive values. (With non-primitives, it
> would crash in runtime.) And consequently, it is used in some places.
> 
> It should be replaced with nsAutoArrayPtr, or UniquePtr.
> 
> Some of the places which are using this:
> https://dxr.mozilla.org/mozilla-central/rev/
> c5da92c5b4906369dee83629f81d647226ac1038/netwerk/system/linux/
> nsNotifyAddrListener_Linux.cpp#162-163
> https://dxr.mozilla.org/mozilla-central/rev/
> c5da92c5b4906369dee83629f81d647226ac1038/ipc/chromium/src/base/
> message_pump_libevent.h#195,211
> https://dxr.mozilla.org/mozilla-central/rev/
> c5da92c5b4906369dee83629f81d647226ac1038/xpcom/tests/gtest/
> TestCloneInputStream.cpp#148
> https://dxr.mozilla.org/mozilla-central/rev/
> c5da92c5b4906369dee83629f81d647226ac1038/dom/media/webspeech/synth/pico/
> nsPicoService.h#85
> https://dxr.mozilla.org/mozilla-central/rev/
> c5da92c5b4906369dee83629f81d647226ac1038/widget/windows/nsWindowGfx.cpp#72
> 
> Probably we should just add a condition to nsAutoPtr that T must not be a
> primitive type. I don't think we would use it to hold a single primitive
> value.

That makes sense to me.  I wonder how much other code there is to fix besides the cases listed above.  Did you get those case from code search or from inserting an appropriate static_assert in nsAutoPtr?
Flags: needinfo?(quanxunzhen)
It was from code search. I can try adding an static_assert and see what will happen.

This bug would conflict with bug 1241518, so if that bug is expected to land very soon, I'll wait for it. Otherwise, that bug may need to wait for me.
Flags: needinfo?(quanxunzhen)
Attachment #8712484 - Flags: review?(nfroyd) → review+
Comment on attachment 8712484 [details]
MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

https://reviewboard.mozilla.org/r/32559/#review29323

Please adds some tests, with const and volatile variants where appropriate, in TestTypeTraits.cpp.

::: mfbt/TypeTraits.h:369
(Diff revision 1)
> + * IsMemberPointer determines whether a type is pointer to non-static member
> + * object or a pointer to non-static member function.

Please move the definition of IsMemberPointer and its helper up in the file to just below IsClass, so that IsMemberPointer's definition comes in the "20.9.4.1 Unary type traits" section.  This way our definitions match up more closely with the standard.
Attachment #8712485 - Flags: review?(nfroyd) → review+
Comment on attachment 8712485 [details]
MozReview Request: Bug 1241901 part 4 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj

https://reviewboard.mozilla.org/r/32561/#review29321

r=me with the changes below.

::: dom/media/webspeech/synth/pico/nsPicoService.cpp:643
(Diff revision 1)
> -    mPicoMemArea = new uint8_t[PICO_MEM_SIZE];
> +    mPicoMemArea = MakeUnique<uint8_t[]>(PICO_MEM_SIZE);

I'm surprised this change compiles, given that UniquePtr no longer converts to T\*.  Don't we have to update uses of mPicoMemArea where appropriate?

Showing B2G builds on your try push shows that this change did indeed break all of them. :)

::: widget/windows/nsWindowGfx.cpp:156
(Diff revision 1)
> -    sSharedSurfaceData = (uint8_t *)malloc(WORDSSIZE(sSharedSurfaceSize) * 4);
> +      MakeUnique<uint8_t[]>(WORDSSIZE(sSharedSurfaceSize) * 4);

Since this allocation could fail before, it needs to use MakeUniqueFallible from mozilla/UniquePtrExtensions.h.
(In reply to Nathan Froyd [:froydnj] from comment #11)
> Comment on attachment 8712484 [details]
> MozReview Request: Bug 1241901 part 2 - Add IsMemberPointer and IsScalar
> type traits.
> 
> ::: mfbt/TypeTraits.h:369
> (Diff revision 1)
> > + * IsMemberPointer determines whether a type is pointer to non-static member
> > + * object or a pointer to non-static member function.
> 
> Please move the definition of IsMemberPointer and its helper up in the file
> to just below IsClass, so that IsMemberPointer's definition comes in the
> "20.9.4.1 Unary type traits" section.  This way our definitions match up
> more closely with the standard.

It depends what standard you are reading...

In C++11, is_member_pointer is the last item in "20.9.4.2 composite type categories" (where I'm currently putting), and in C++14, it is the last item in "20.10.4.2 composite type categories".
*depends on
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> (In reply to Nathan Froyd [:froydnj] from comment #11)
> > Please move the definition of IsMemberPointer and its helper up in the file
> > to just below IsClass, so that IsMemberPointer's definition comes in the
> > "20.9.4.1 Unary type traits" section.  This way our definitions match up
> > more closely with the standard.
> 
> It depends what standard you are reading...
> 
> In C++11, is_member_pointer is the last item in "20.9.4.2 composite type
> categories" (where I'm currently putting), and in C++14, it is the last item
> in "20.10.4.2 composite type categories".

You're right!  I was getting it confused with is_member_object_pointer.  Please excuse my error, and the patch is fine as is...once you add the tests, anyway. :)
Attachment #8712483 - Flags: review?(mcmanus) → review?(daniel)
Comment on attachment 8712483 [details]
MozReview Request: Bug 1241901 part 1 - Remove nsAutoPtr uses in nsNotifyAddrListener on Linux. r=bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32557/diff/1-2/
Attachment #8712483 - Flags: review?(daniel) → review?(mcmanus)
Comment on attachment 8712484 [details]
MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32559/diff/1-2/
Comment on attachment 8712485 [details]
MozReview Request: Bug 1241901 part 4 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32561/diff/1-2/
Attachment #8712485 - Attachment description: MozReview Request: Bug 1241901 part 3 - Stop using nsAutoPtr for holding primitive arrays. → MozReview Request: Bug 1241901 part 3 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj
Attachment #8712483 - Flags: review?(mcmanus) → review?(daniel)
Comment on attachment 8712484 [details]
MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

Please review the tests.
Attachment #8712484 - Flags: review+ → review?(nfroyd)
Well, I tried to not break b2g build... but... it seems someone is really using nsAutoPtr to hold pointer to a single scalar value...
Comment on attachment 8712483 [details]
MozReview Request: Bug 1241901 part 1 - Remove nsAutoPtr uses in nsNotifyAddrListener on Linux. r=bagder

https://reviewboard.mozilla.org/r/32557/#review29551

Looks good to me!
Attachment #8712483 - Flags: review?(daniel) → review+
Comment on attachment 8712483 [details]
MozReview Request: Bug 1241901 part 1 - Remove nsAutoPtr uses in nsNotifyAddrListener on Linux. r=bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32557/diff/2-3/
Attachment #8712483 - Attachment description: MozReview Request: Bug 1241901 part 1 - Remove nsAutoPtr uses in nsNotifyAddrListener on Linux. → MozReview Request: Bug 1241901 part 1 - Remove nsAutoPtr uses in nsNotifyAddrListener on Linux. r=bagder
Comment on attachment 8712484 [details]
MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32559/diff/2-3/
Attachment #8712484 - Attachment description: MozReview Request: Bug 1241901 part 2 - Add IsMemberPointer and IsScalar type traits. → MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.
Attachment #8712485 - Attachment description: MozReview Request: Bug 1241901 part 3 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj → MozReview Request: Bug 1241901 part 4 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj
Comment on attachment 8712485 [details]
MozReview Request: Bug 1241901 part 4 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32561/diff/2-3/
Comment on attachment 8713126 [details]
MozReview Request: Bug 1241901 part 2 - Use intptr_t to pass bluetooth service class instead of a pointer to heap.

Redirect review to module owner Shawn since I'm unfamiliar with dbus.
Attachment #8713126 - Flags: review?(btian) → review?(shuang)
ni? requester to change reviewer per comment 29.

xidorn, can you help change reviewer to Shawn (shuang) on mozReview? Thanks.
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(quanxunzhen)
Comment on attachment 8713126 [details]
MozReview Request: Bug 1241901 part 2 - Use intptr_t to pass bluetooth service class instead of a pointer to heap.

https://reviewboard.mozilla.org/r/32745/#review29647

It looks good to me.
Attachment #8713126 - Flags: review?(shuang) → review+
Comment on attachment 8712484 [details]
MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

https://reviewboard.mozilla.org/r/32559/#review29659
Attachment #8712484 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7309a6dbb31c56a827dcc8ebe20f32840f8e16db
Bug 1241901 part 1 - Remove nsAutoPtr uses in nsNotifyAddrListener on Linux. r=bagder

https://hg.mozilla.org/integration/mozilla-inbound/rev/874a8f916268e26ef40b834f1894d991f57d6278
Bug 1241901 part 2 - Use intptr_t to pass bluetooth service class instead of a pointer to heap. r=shawnjohnjr

https://hg.mozilla.org/integration/mozilla-inbound/rev/30ded57e0902f7767c8bcd3975b78cf22b08437e
Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/673a9080e14d7537aabdf19475df2d655bc240b7
Bug 1241901 part 4 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj
Assignee: nobody → quanxunzhen
Blocks: 1244466
Comment on attachment 8713126 [details]
MozReview Request: Bug 1241901 part 2 - Use intptr_t to pass bluetooth service class instead of a pointer to heap.

https://reviewboard.mozilla.org/r/32745/#review123336
You need to log in before you can comment on or make changes to this bug.