420358 - firefox does not check return value of close() when downloading a file

Status: NEW

(Toolkit :: Downloads API, defect, P3)

Description

[Jeff Bastian]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.12) Gecko/20080208 Fedora/2.0.0.12-1.fc8 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.12) Gecko/20080208 Fedora/2.0.0.12-1.fc8 Firefox/2.0.0.12

The Firefox download manager does not check the return value of close() for errors, e.g. ENOSPC, when saving a file.  Normally this is not a problem because it does check write() for errors as it's downloading.  However, some filesystems do not return errors on write() even if the disk is full, e.g., SMB/CIFS.  SMB/CIFS does return an error on close(), though.

So, if you're downloading a large file and saving it to a SMB/CIFS mount, and that filesystem is nearly full, you will not get an error message from Firefox, i.e., it appears to download successfully.  When you try to read the file, though, it's corrupt (truncated).

To test this, I wrote a short program to create a 10MB file, 8192 bytes at a time, and check for errors on both write() and close().  On a local ext2 filesystem, it stopped with an error on write() when the filesystem was full, but on a CIFS mount, write() succeeded every time and only close() received an error.

   $ df -h /mnt/local /mnt/samba
   Filesystem            Size  Used Avail Use% Mounted on
   /tmp/file.img         4.9M   33K  4.6M   1% /mnt/local
   //192.168.1.74/samba  4.9M   34K  4.6M   1% /mnt/samba

   $ ./ten-mb /mnt/local/foo
   8192 bytes written (8192 total)...
   8192 bytes written (16384 total)...
   ...
   8192 bytes written (4767744 total)...
   8192 bytes written (4775936 total)...
   errno = 28
   write error: No space left on device

   $ ./ten-mb /mnt/samba/foo
   8192 bytes written (8192 total)...
   8192 bytes written (16384 total)...
   ... 
   8192 bytes written (10477568 total)...
   8192 bytes written (10485760 total)...
   errno = 28
   close error: No space left on device

As you can see, the write() calls succeeded all the way to 10MB even though the CIFS filesystem was full at about 4.7MB.

Since not all filesystems are created equal, Firefox should check the value of close() for errors too.

Reproducible: Always

Steps to Reproduce:
1. Use Firefox to download a large file
2. Save the file to a SMB/CIFS mount that's nearly full (less free space than the file being downloaded)

Actual Results:  
Firefox appears to download the file successfully and completely, however, the downloaded file is silently truncated when the filesystem is full.

Expected Results:  
Firefox should report "Error: No space left on device"

Comment 1

[Jeff Bastian]

Attachment: test program to create 10MB file

Usage:
  gcc -o ten-mb ten-mb.c
  ./ten-mb /path/to/file

ten-mb will write null bytes to the output file 8192 bytes at a time up to
10MB.

Comment 2

[Jeff Bastian]

On a local ext3 filesystem, Firefox does give an error, but it's misleading:
  There is not sufficient memory to complete the action you requested.
  Quit some applications and try again.

There was plenty of free memory, but the disk was full.

Comment 3

[Shawn Wilsher :sdwilsh]

-> Core:File Handling?

Note: this probably won't get fixed on 1.8.1 branch (firefox 2.0.0.*), but has a chance for 1.9 (firefox 3.0.*).

Comment 4

[Jeff Bastian]

I did test on Firefox 3.0b3 and it still has this problem.

Comment 5

[Jeff Bastian]

From the Linux close(2) man page:

  NOTES
       Not  checking  the return value of close() is a common but nevertheless
       serious programming error.  It is quite possible that errors on a  pre-
       vious  write(2) operation are first reported at the final close().  Not
       checking the return value when closing the file may lead to silent loss
       of data.  This can especially be observed with NFS and with disk quota.

Comment 6

[Jesse Ruderman]

I wonder whether we make the same mistake with profile data files.  Corrupting those is worse than corrupting a download, since you can usually download a file again.

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to comment #6)
> I wonder whether we make the same mistake with profile data files.  Corrupting
> those is worse than corrupting a download, since you can usually download a
> file again.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit BugBot documentation.

Comment 10

[Marco Bonardo [:mak]]

We should be handling the errors on close() today, though it would be worth verifying how it is being handled on the user side.