Bug 3303 - Renamed file with DELETE_ON_CLOSE removes OLD-named file on close
Renamed file with DELETE_ON_CLOSE removes OLD-named file on close
Status: RESOLVED FIXED
Product: Samba 3.0
Classification: Unclassified
Component: File Services
3.0.20b
All All
: P3 critical
: none
Assigned To: Samba Bugzilla Account
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-06 09:26 UTC by SATOH Fumiyasu
Modified: 2005-12-14 10:46 UTC (History)
1 user (show)

See Also:


Attachments
ad-hoc test code (8.02 KB, text/x-csrc)
2005-12-06 09:41 UTC, SATOH Fumiyasu
no flags Details
proposed patch to fix removing old name file on DELETE_ON_CLOSE (5.82 KB, patch)
2005-12-10 06:24 UTC, SATOH Fumiyasu
no flags Details
proposed patch to fix removing old name file on DELETE_ON_CLOSE (6.01 KB, patch)
2005-12-10 06:44 UTC, SATOH Fumiyasu
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description SATOH Fumiyasu 2005-12-06 09:26:07 UTC
Problem:
--------
1. If file is renamed from client, new filename is propagated
    to file handlers (files_struct *fsp) opening renamed file
    in SAME smbd process only. File handers opening renamed file
    in OTEHR smbd processes are not affected.
2. DELETE_ON_CLOSE flag removes file based on filename.

How to cause a problem on the real world:
-----------------------------------------
Prepare:
    Put a MS-Excel file (e.g. file.xls) into a share on Samba server.
Client1:
    Open file.xls with read/write mode.
Client2:
    Open file.xls with read-only mode.
Client1:
    Do "Save" on MS-Excel:
        Save new data to temporary file (e.g. file.new).
        Rename file.xls to temporary file (e.g. file.old).
        Rename file.new to file.xls.
        Set DELETE_ON_CLOSE flag to file.old.
    Close file.old (renamed old file.xls with DELETE_ON_CLOSE flag).
        But file.old is NOT removed here because it is opened by Client2 yet.
Client2:
    Close file.old (renamed old file.xls with DELETE_ON_CLOSE flag).
        file.old is removed now.
        file.xls is NOT removed.
Comment 1 SATOH Fumiyasu 2005-12-06 09:39:24 UTC
(In reply to comment #0)
> Client2:
>     Close file.old (renamed old file.xls with DELETE_ON_CLOSE flag).
>         file.old is removed now.
>         file.xls is NOT removed.

This (correct) result is occured on Windows share.
But on Samba share, file.old is remained and file.xls is removed.
Comment 2 SATOH Fumiyasu 2005-12-06 09:41:07 UTC
Created attachment 1606 [details]
ad-hoc test code

Compile:
--------
$ tar xzf samba-3.0.20b.tar.gz
$ cd samba-3.0.20b/source
$ ./configure && make
$ gcc -I. -Iinclude -Iubiqx -Ipopt -c /PATH/TO/t_del_on_close.c
$ gcc -o t_del_on_close `sed -n 's/^LIBS=//p' Makefile` t_del_on_close.o bin/libsmbclient.a

Setup:
------
1. Make smb.conf:
  [share]
  path = /project/share
  write ok = yes
2. Put file.xls into share dir.
  $ touch /project/share/file.xls
3. Start smbd.

Test:
-----
$ ./t_del_on_close 127.0.0.1 share user password
Comment 3 Volker Lendecke 2005-12-06 10:52:12 UTC
Thanks for that detailed report. I'll see if I can reproduce it and try to find a fix. This will probably end up as another test in the Samba4 raw-delete test.

Volker
Comment 4 SATOH Fumiyasu 2005-12-10 06:24:18 UTC
Created attachment 1608 [details]
proposed patch to fix removing old name file on DELETE_ON_CLOSE

This patch fixes the following problems:

* Remove file based on old filename by DELETE_ON_CLOSE flag
  for renamed file.
* smbstatus(1) and SWAT shows old filename in
  status information for renamed file.

This patch does NOT fix the following problems:

* smbd(8) shows old filename in log file
  for renamed filename.
* File processing (except remove on DELETE_ON_CLOSE case) based on
  filename for opened and renamed file access to
  old filename.
Comment 5 SATOH Fumiyasu 2005-12-10 06:44:28 UTC
Created attachment 1609 [details]
proposed patch to fix removing old name file on DELETE_ON_CLOSE

Updated.
Comment 6 Jeremy Allison 2005-12-10 09:43:28 UTC
Thanks - this is pretty close to what I'm working on. The issue you've missed is if a rename takes place on a different share that is mapped to the same area of the filesystem. This then could cause data loss also. The original filename stored in the locking.tdb before the recent rewrite was a complete pathname on the system not a pathname relative to the share - we'll have to go back to that to protect against this issue (there is no perfect solution to this issue but at least we need to detect it).
Jeremy.
Comment 7 Jeremy Allison 2005-12-13 11:11:48 UTC
Fixed in SAMBA_3_0 SVN and HEAD. Please check out and test. This is a generic fix that will fix the get delete on close trans2 calls also.
Jeremy.
Comment 8 SATOH Fumiyasu 2005-12-14 03:58:03 UTC
Looks like bad... There is a race condition.
From SMB_VFS_RENAME() to rename_share_filename() must be atomic.

Otherwise, 

 1. smbd A: SMB_VFS_RENAME()
 2. smbd B: lck = get_share_mode_lock()
 3. smbd A: lck = get_share_mode_lock() - wait for smbd B...
 4. smbd B: Try to do something to file based on filename,
            but failed (or ocurred another problem?).
 5. smbd B: talloc_free(lck)
 6. smbd A: continued...

I think smbd must do `lck = get_share_mode_lock()' before
SMB_VFS_RENAME() and do `talloc_free(lck)' after
rename_open_files().
Comment 9 Jeremy Allison 2005-12-14 09:16:54 UTC
This (rename) is inherently racy. Your modification would reduce the race, but not remove it, that's why I coded it this way instead of your way.
The message passing code is not synchronous - there is no guarentee that the receiving smbd will have processed the message or not be in the close code at the point the message is sent. As tdb_chainlock and tdb_chainunlock are recursion safe within one smbd I can increate the lock region to reduce the race, but this *always* has the potential to fail - that's why there's the check in smbd/close.c
Jeremy.
Comment 10 Jeremy Allison 2005-12-14 10:46:43 UTC
Check out the change I made to hold the locks longer.
Jeremy.