Convert raw pointers in DNS.cpp and nsHostResolver.cpp to smart pointers

RESOLVED DUPLICATE of bug 1420677

Status

()

P3
normal
RESOLVED DUPLICATE of bug 1420677
4 years ago
9 days ago

People

(Reporter: valentin, Unassigned, Mentored)

Tracking

41 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
There are a lot of raw pointers being manually managed and passed around functions. Changing them to nsAutoPtr would be very helpful to prevent leaks and make the code easier to follow.
(In reply to Valentin Gosu [:valentin] from comment #0)
> There are a lot of raw pointers being manually managed and passed around
> functions. Changing them to nsAutoPtr would be very helpful to prevent leaks
> and make the code easier to follow.

Hi Valentin, I'm interested in this bug. I don't really understand the working of nsAutoPtr, so I would like to know whether such steps would work :-
char *a; // write this as nsAutoPtr<char> a;

Would I need to make any changes in the dereference part?
Hi Valentin, I realized that you weren't in the CC list. Is this bug defunct, or I could still work on it?
(Reporter)

Comment 3

4 years ago
Hi Kalpesh,

This bug is definitely still valid.
Replacing char*a with nsAutoPtr<char> a; should usually be enough, but you cave to take care when passing the pointer to other functions. If that function saves a copy, or passes it to another thread, this would be a big problem.
In that case we would probably have to change AddrInfo and other classes to extend nsISupports and use nsRefPtr when storing the pointers.
Alright so how should I begin? Should I just write all the pointers of primitive data types to nsAutoPtr for a start? Also, do I need to write pointers of the types like NetAddr http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/DNS.cpp#139 as nsAutoPtr as well?
(Reporter)

Comment 5

4 years ago
If the function just uses the pointer to access the class members, then a primitive with a raw pointer is OK. If it saves the pointer to another structure, or passes it to another thread, then I think the primitive needs to pass a reference to the nsAutoPtr.

I think NetAddr should be converted as well, but I have the feeling that simply using nsAutoPtr isn't going to work. You're free to try though.

Comment 6

4 years ago
Hi I would like to work on this bug is this still valid?
(Reporter)

Comment 7

4 years ago
(In reply to Jinank Jain from comment #6)
> Hi I would like to work on this bug is this still valid?

Yes, it's still valid.
Whiteboard: [necko-backlog]
Posted patch bug1185120_ver1.patch (obsolete) — Splinter Review
There's no new updates since one year ago. I guess others are not working on it anymore? I'd like to fix this, too. Here's my first try.
Posted patch bug1185120_ver2.patch (obsolete) — Splinter Review
Version 2, fix a possible segmentation fault.
Attachment #8785648 - Attachment is obsolete: true
Hi Valentin, does this look OK or I need to change more codes?
Flags: needinfo?(valentin.gosu)
(Reporter)

Comment 11

3 years ago
The patch looks good. Could you specify what the possible seg fault was in ver1? I didn't notice any difference between the patches by visual observation :)

I previously tried to fix this in bug 1183781, which resulted in bug 1186435. It might have been the same issue.
Flags: needinfo?(valentin.gosu)
In version1 there's a line:

!strlen(mHostRecord->addr_info->mCanonicalName.get())

I change it to

!mHostRecord->addr_info->mCanonicalName.get()

At least on GNU libc, strlen crashes with SIGSEGV if fed with NULL pointers. I didn't observe a segmentation fault in real cases, though.
(Reporter)

Comment 13

3 years ago
Comment on attachment 8785649 [details] [diff] [review]
bug1185120_ver2.patch

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

I think this is right.

also remove these lines from build/sanitizers/lsan_suppressions.txt - hopefully this patch fixes the leaks.
> leak:nsDNSService::AsyncResolveExtended	
> leak:_GetAddrInfo_Portable

And do a try run before landing.

::: netwerk/dns/nsHostResolver.cpp
@@ +1299,5 @@
>  
>          rec->negative = !rec->addr_info;
>          PrepareRecordExpiration(rec);
>          rec->resolving = false;
>          

Let's remove the whitespace since we're changing this code.
Attachment #8785649 - Flags: review+
Well I don't have access to try servers. Need I open another request for applying permissions?
(Reporter)

Updated

3 years ago
Assignee: nobody → yan12125
(Reporter)

Comment 16

3 years ago
I pushed to try for you.
You might be interested in applying for Level 1 commit access. See bug 1116226 as an example.

Thanks!
Thanks, I'm looking into the procedure of application.
(Reporter)

Comment 18

3 years ago
It seems we have a build error on windows.
Please fix and do another try run.
https://treeherder.mozilla.org/logviewer.html#?job_id=26845614&repo=try#L13580
Flags: needinfo?(yan12125)
Yeah, and I need to install Windows first...
(Reporter)

Comment 20

3 years ago
I think you just need to make sure that other uses of mCanonicalName and mHostName in other files also use .get() / maybe the same is needed for addr and addr info.
Thanks updated. Seems my patch is OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05bedb7fa2a2. Could you review this?
Attachment #8785649 - Attachment is obsolete: true
Flags: needinfo?(yan12125) → needinfo?(valentin.gosu)
(Reporter)

Comment 22

3 years ago
Comment on attachment 8788056 [details] [diff] [review]
bug1185120_ver3.patch

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

Thanks for the patch.
Attachment #8788056 - Flags: review+
(Reporter)

Updated

3 years ago
Flags: needinfo?(valentin.gosu)
Keywords: checkin-needed

Comment 23

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38cad72a77a6
Convert raw pointers in DNS.cpp and nsHostResolver.cpp to smart pointers. r=valentin
Keywords: checkin-needed

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/38cad72a77a6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This was backed out for causing bug 1301069.
Status: RESOLVED → REOPENED
status-firefox51: fixed → ---
Depends on: 1301069
No longer depends on: 1301180
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---

Updated

a year ago
Depends on: 1417827

Updated

a year ago
Depends on: 1420673

Updated

a year ago
Depends on: 1420677
Assignee: yan12125 → nobody

Sorry I don't think I have enough knowledge to write a proper fix, so I unassigned myself to clearly indicate that everyone interested can work on it.

(Reporter)

Updated

9 days ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago9 days ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1420677
You need to log in before you can comment on or make changes to this bug.