1053277 - Add log of invalid fd close

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #1038461 +++

Close invalid fd close could close valid fd if fd is already reused. It could cause problems like Bug 1038461 and Bug 1047149.

It seems better to logout when fd close fails.

Comment 1

[Sotaro Ikeda [:sotaro]]

attachment 8472044 [details] [diff] [review] in Bug 1038461 did it by modifying bionic. It requests to modify bionic and is not a ideal way to do. It seems better that close() is intercepted by gecko like malloc().

Comment 2

[Sotaro Ikeda [:sotaro]]

mwu, how do you think about what is a good way to add "invalid fd close log"?

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> attachment 8472044 [details] [diff] [review] in Bug 1038461 did it by
> modifying bionic. It requests to modify bionic and is not a ideal way to do.
> It seems better that close() is intercepted by gecko like malloc().

But libc is sometimes used as static lib. If want to check this case, modification to bionic seems unavoidable.

Comment 4

[Michael Wu [:mwu]]

We can put something in mozglue for debug builds to override the default close() implementation. Then, when one attempts to close an invalid FD, assert.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: dhcpd log

outside of b2g related process, there are some process that try to close invaild fds. It seems that they are intentional. On example is dhcpd.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

[Blocking Requested - why for this release]:

Comment 7

[bhavana bajaj [:bajaj]]

:milan, I am clearing the blocking flag here as this is just a logging bug, feel free to seek approval if this needs to be fixed on 2.1 or NI me if I am missing info for making a blocking call here.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm worried that it isn't going to even get looked at if it isn't 2.1+.  And if it doesn't get resolved, we run a chance of hitting the FD related bugs, like we have in the past, and those are difficult to find (take hours of testing) and easy to assign to the wrong area, because they cause a crash far away from the culprit.  So, non-2.1+, assigned to FirefoxOS->General having likely a zero chance of getting traction is why I nominated this in the first place, but if you can make it happen I don't care if it's 2.1+ :)

Comment 9

[bhavana bajaj [:bajaj]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> I'm worried that it isn't going to even get looked at if it isn't 2.1+.  And
> if it doesn't get resolved, we run a chance of hitting the FD related bugs,
> like we have in the past, and those are difficult to find (take hours of
> testing) and easy to assign to the wrong area, because they cause a crash
> far away from the culprit.  So, non-2.1+, assigned to FirefoxOS->General
> having likely a zero chance of getting traction is why I nominated this in
> the first place, but if you can make it happen I don't care if it's 2.1+ :)

starting with :mwu here, to see if he can help with comment #4?

Comment 10

[Michael Wu [:mwu]]

I can help someone figure out how to do this, if we have someone free.

Comment 11

[Sotaro Ikeda [:sotaro]]

I already created some temporary ptached to investigate Bug 1057220. [1] is simple and only affect to gecko. Though it could not check the invalid fd close happen out of gecko like Bug 1058366. 

-[1] add log of close() called only from gecko
   + attachment 8477861 [details] [diff] [review] in Bug 1057220
-[2] add log of close() to bionic on gonk KK
   + part 1 KK bionic: attachment 8477910 [details] [diff] [review] in Bug 1057220
   + part 2 gecko: attachment 8477911 [details] [diff] [review] in Bug 1057220
-[3] add log of close() to bionic on gonk JB
   + part 1 JB bionic: attachment 8478471 [details] [diff] [review] in Bug 1054929
   + part 2 gecko: attachment 8478473 [details] [diff] [review] in Bug 1054929

Comment 12

[Michael Wu [:mwu]]

The approach in [1] is what I'm looking for, though you might want to use __real_close to call close rather than using syscall. Not sure if that works on B2G though - the only option there might be to directly load the symbol. Also should only be enabled on debug builds.

This will actually work on libraries outside of gecko as long as the library is not bionic. We use LD_PRELOAD to load this code which forces all code to use the version of close.

Comment 14

[BMO Automation]

Firefox OS is not being worked on