1175387 - libmdns block on recvfrom()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

This is a follow-up for bug 1171827.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

I encountered this bug again. After debugging mdnsd, I found that it error handling logic and mdnsd enters an infinite loop:

* In external/mdnsresponder/mDNSPosix/mDNSPosix.c::mDNSPosixRunEventLoopOnce(), it calls select(). select() returns -1 and errno = EBADF. One of the file descriptor being watched has an error. It notifies its caller that there are no data dispatched.
* In external/mdnsresponder/mDNSPosix/PosixDaemon.c::MainLoop(), continues to the next iteration, with gotData == false and enter mDNSPosixRunEventLoopOnce(). Because EBADF was not handled, select() will still return -1 and EBADF. This becomes a CPU spin without any progress.

We need to upgrade mdnsresponder to see if this can be fixed in a newer version.

Comment 2

[:garvan]

Currently have this happening on the Sony foxfooding device. I had a 20 minute period where:
- first was slow to wake
- took 30 secs to respond to each tap
- after device went back to sleep, could not wake it with the power button, device was hot, adb reported 80% CPU consumption of root b2g process. 

While reading bugs about this for 5-10 mins, CPU consumption suddenly went back to zero, and device back to normal.

Comment 3

[Doug Sherk (:drs) (inactive)]

Gary, Cervantes, are there any updates on this? Based on comment 2 and other reports, foxfooders are still facing this issue. This is a very bad bug.

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

We may need to patch this our own and upstream the fix. We are already using master branch of mdnsresponder and it hasn't been updated for ~3yrs.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Cervantes Yu from comment #4)
> We may need to patch this our own and upstream the fix. We are already using
> master branch of mdnsresponder and it hasn't been updated for ~3yrs.

Yes, let's please do that :)

Comment 6

[Gary Chen [:xeonchen]]

We don't know what triggers this issue, which makes it difficult to investigate and verify.
The library is also used on Android and Apple's devices, but they don't have this issue?

Garvan, do you mean your device went back to normal without any action?
Did you change the network status before the situation happened? E.g. enter basement/elevator
which cause cellular/WiFi disconnect?

Comment 7

[:garvan]

> Garvan, do you mean your device went back to normal without any action?

Yes, I was running adb top in a terminal, and then noticed the CPU usage had dropped, I picked up the device and it was responding again. The entire time, the device was on my desk (from when I noticed the problem to when it was resolved).

> Did you change the network status before the situation happened? E.g. enter
> basement/elevator
> which cause cellular/WiFi disconnect?

No.

I am sure other foxfooders will have this happen, we should let the list know we are looking for more feedback on the events surrounding this issue.

Comment 8

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1175387-close-file-descriptor-after-detached-fro.patch

The file descriptor owned by |DNSServiceRef| is closed before being detached from STS. It may cause |EBADF| while polling on a closed descriptor.

Besides, the descriptor is closed twice by |DNSServiceRefDeallocate| and |PR_Close|, so I use |PR_CreateSocketPollFd| instead.

Patrick, would you give some comment on these modifications?

Comment 9

[Patrick McManus [:mcmanus]]

(In reply to Gary Chen [:xeonchen] from comment #8)

> Besides, the descriptor is closed twice by |DNSServiceRefDeallocate| and
> |PR_Close|, so I use |PR_CreateSocketPollFd| instead.

we definitely need to backport any fix becuase of that. It could accidentally close another (unrelated socket - opened after the first close).

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8628111 [details] [diff] [review]
0001-Bug-1175387-close-file-descriptor-after-detached-fro.patch

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

::: netwerk/dns/mdns/libmdns/MDNSResponderOperator.cpp
@@ +122,5 @@
>      if (osfd == -1) {
>        return NS_ERROR_FAILURE;
>      }
>  
> +    mFD = PR_CreateSocketPollFd(osfd);

to be honest, I've never used that bit of nspr.. but the documentation says the result is only for use with pr_poll() and you're doing way more than that - so I don't think it is a good idea.

I think you can continue with the old importFile approach, but just before the pr_close() do PR_ChangeFileDescNativeHandle() to change the osfd to -1.

Comment 11

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1175387-close-file-descriptor-after-detached-fro.patch

Close fd after detached from STS, and make PR_Close does nothing.

Comment 12

[Gary Chen [:xeonchen]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 14

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1175387-close-file-descriptor-after-detached-fro.patch

rebase, carry r+

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/1f6f47627dbd

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f6f47627dbd