Opened 4 years ago

Closed 4 years ago

#15857 closed defect (fixed)

File descriptor leak in Firefox updater causes update failures

Reported by: mikeperry Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: TorBrowserTeam201505R
Cc: mcs, brade, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I'm getting some strange errors when running the updater on a very hacked-up version of Tor Browser on Linux. In one instance, I got "ensure_copy_recursive: path is not a directory: /home/user/tor-browser_en-US/Browser/TorBrowser/Data/Browser/profile.default/Cache/4/99, rv: 0, err: 24". In another (when I enabled app.update.logs) I got:

*** AUS:SVC Downloader:onProgress - progress: 53654724/53654724
*** AUS:SVC Downloader:onStopRequest - original URI spec: https://www.torproject.org/dist/torbrowser/4.5/tor-browser-linux64-4.5_en-US.mar, final URI spec: https://dis
t.torproject.org/torbrowser/4.5/tor-browser-linux64-4.5_en-US.mar, status: 0
*** AUS:SVC Downloader:onStopRequest - status: 0, current fail: 0, max fail: 10, retryTimeout: 2000
*** AUS:SVC Downloader:_verifyDownload called
*** AUS:SVC Downloader:_verifyDownload downloaded size == expected size.
*** AUS:SVC Downloader:_verifyDownload hashes do not match. 
*** AUS:SVC Downloader:onStopRequest - download verification failed
*** AUS:SVC getStatusTextFromCode - transfer error: The integrity of the update could not be verified, code: verification_failed
*** AUS:SVC Downloader:onStopRequest - setting state to: download-failed
*** AUS:SVC Downloader:onStopRequest - all update patch downloads failed

Oddly, while trying to reproduce this more times, the update finally downloaded successfully and applied fine. This caused me to suspect a bad mirror, but that full MAR file seems fine on all mirrors.

I am out of ideas, and I have not been able to reproduce this issue on any vanilla TBB on the same systems, even if I force it to do a full update by editing the start-tor-browser script.

Child Tickets

Change History (10)

comment:1 Changed 4 years ago by mikeperry

Cc: brade added

comment:2 Changed 4 years ago by mikeperry

Summary: Strange errors with Linux updaterStrange errors with full Linux update when disk records are enabled

I haven't yet tried enabling disk records in a vanilla TBB and forcing a full update. The disk cache might be a factor here somehow, for both errors. Setting the title anyway, to better describe the situation where it did happen.

comment:3 Changed 4 years ago by gk

Cc: gk added

comment:4 Changed 4 years ago by mcs

The first error you encountered (ensure_copy_recursive: path is not a directory) is probably caused by leaking of file descriptors. "err: 24" is EMFILE. Looking at the code, we found that closedir() is not being called during a recursive copy inside the updater. We filed a Mozilla bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1159826

We will do a test where we enable browsing history in TB 4.5a5 and modify start-tor-browser to force a full update.

comment:5 in reply to:  4 Changed 4 years ago by mcs

Replying to mcs:

We will do a test where we enable browsing history in TB 4.5a5 and modify start-tor-browser to force a full update.

This succeeded. Maybe the second error can be explained by a download error? Somewhat surprising though.

Version 0, edited 4 years ago by mcs (next)

comment:6 Changed 4 years ago by mcs

Status: newneeds_information

Mozilla has merged the file descriptor leak fix. See https://hg.mozilla.org/releases/mozilla-esr38/rev/9edf93465d0d

Kathy and I think we should take this fix for TB 4.5.next since it is trivial and may avoid some future update problems. Mike and Georg, do you agree?

comment:7 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201505R added

comment:8 in reply to:  6 ; Changed 4 years ago by gk

Replying to mcs:

Mozilla has merged the file descriptor leak fix. See https://hg.mozilla.org/releases/mozilla-esr38/rev/9edf93465d0d

Kathy and I think we should take this fix for TB 4.5.next since it is trivial and may avoid some future update problems. Mike and Georg, do you agree?

Sounds good to me. But what bothers me is why we are touching this part of the user profile at all. The extensions directory, granted. But all the other things should be left alone IMO.

comment:9 in reply to:  8 Changed 4 years ago by mcs

Replying to gk:

Sounds good to me. But what bothers me is why we are touching this part of the user profile at all. The extensions directory, granted. But all the other things should be left alone IMO.

When using "staging" (aka "apply an update and then ask the user to restart their browser"), the updater makes a recursive copy of everything before it tries to apply an update. We could try to be selective and not copy the parts that are purely user data, but that would be a little tricky (we would have to move things into the right places at the right time, and there is no code in Mozilla's updater to do that since their profile data is elsewhere).

comment:10 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_informationclosed
Summary: Strange errors with full Linux update when disk records are enabledFile descriptor leak in Firefox updater causes update failures

Ok, I took this patch for 4.5.1. I will file a new bug for the hash issue if it happens again after this fix is deployed. It may have been a mirror syncing issue that I just did not inspect quick enough for it to still be there.

Note: See TracTickets for help on using tickets.