1241901 - nsAutoPtr should not be used to hold array

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

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)()

Comment 2

[Gerald Squelart (he/him)]

(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!(?)

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

That's good news. I'll leave some messages there.

Comment 4

[Nathan Froyd [:froydnj]]

(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?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3c53e2e0f9

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73b5aa0001b0

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1241901 part 1 - Remove nsAutoPtr uses in nsNotifyAddrListener on Linux. r=bagder

Review commit: https://reviewboard.mozilla.org/r/32557/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32557/

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

Review commit: https://reviewboard.mozilla.org/r/32559/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32559/

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1241901 part 4 - Stop using nsAutoPtr for holding primitive arrays. r=froydnj

Review commit: https://reviewboard.mozilla.org/r/32561/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32561/

Comment 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

[Nathan Froyd [:froydnj]]

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.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

(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".

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

*depends on

Comment 15

[Nathan Froyd [:froydnj]]

(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. :)

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

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/

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

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 18

[Xidorn Quan [:xidorn] UTC+11]

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/

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8712484 [details]
MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

Please review the tests.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05510af1f5ad

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

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 22

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 24

[Daniel Stenberg [:bagder]]

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!

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

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/

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1241901 part 2 - Use intptr_t to pass bluetooth service class instead of a pointer to heap.

Review commit: https://reviewboard.mozilla.org/r/32745/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32745/

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

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/

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

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 29

[Ben Tian (inactive)]

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.

Comment 30

[Ben Tian (inactive)]

ni? requester to change reviewer per comment 29.

xidorn, can you help change reviewer to Shawn (shuang) on mozReview? Thanks.

Comment 31

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 8712484 [details]
MozReview Request: Bug 1241901 part 3 - Add IsMemberPointer and IsScalar type traits.

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

Comment 33

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7309a6dbb31c
https://hg.mozilla.org/mozilla-central/rev/874a8f916268
https://hg.mozilla.org/mozilla-central/rev/30ded57e0902
https://hg.mozilla.org/mozilla-central/rev/673a9080e14d

Comment 35

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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