Bug 7863 - Unlink may unlink wrong file when hardlinks are involved
Unlink may unlink wrong file when hardlinks are involved
Status: ASSIGNED
Product: Samba 3.2
Classification: Unclassified
Component: File services
unspecified
All FreeBSD
: P3 major
: ---
Assigned To: Volker Lendecke
Samba QA Contact
http://mercurial.selenic.com/bts/issu...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-13 11:07 UTC by Matt Mackall
Modified: 2010-12-22 18:57 UTC (History)
1 user (show)

See Also:


Attachments
client side wireshark trace (10.95 KB, application/octet-stream)
2010-12-22 14:17 UTC, Adrian Buehlmann
no flags Details
server side level 10 smbd log for that client (kork), zipped (21.27 KB, application/octet-stream)
2010-12-22 14:20 UTC, Adrian Buehlmann
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Mackall 2010-12-13 11:07:36 UTC
If a member of a hardlink pair is held open, it may be wrongly deleted if you attempt to remove the OTHER member.

See the URL for the full bug report in Mercurial's BTS.
Comment 1 Adrian Buehlmann 2010-12-13 12:14:43 UTC
As noted in the Mercurial BTS entry, I was able to reproduce the bug with
latest stable Samba 3.5.6 (compiled from source on Ubuntu Linux 10.10 Maverick).

Note that the required file open (as done with mercurial.windows.posixfile)
opens the file with FILE_SHARE_DELETE for the win32 API CreateFile
( http://selenic.com/repo/hg/file/bf763946f8b0/mercurial/osutil.c#l476 )
Comment 2 Jeremy Allison 2010-12-13 12:23:50 UTC
Please attach a wireshark trace and also debug level 10 log from smbd during the client activity.

Thanks,

Jeremy.
Comment 3 Adrian Buehlmann 2010-12-22 14:17:14 UTC
Created attachment 6162 [details]
client side wireshark trace
Comment 4 Adrian Buehlmann 2010-12-22 14:20:21 UTC
Created attachment 6163 [details]
server side level 10 smbd log for that client (kork), zipped
Comment 5 Adrian Buehlmann 2010-12-22 14:57:27 UTC
Steps recorded in those trace-/log-files:

On the client:

1) create file 'a.txt' on the share
2) hardlink 'a.txt' to 'a.link.txt'
3) open 'a.txt' with FILE_SHARE_DELETE share mode
4) delete 'a.link.txt'
5) close 'a.txt'

After that, file 'a.txt' was missing (!) and file 'a.link.txt' was still there (which is not expected).

Client: Windows 7 Ultimate x64
Server: Samba 3.5.6 (compiled from source) on Ubuntu Linux 10.10
Comment 6 Adrian Buehlmann 2010-12-22 15:14:13 UTC
(In reply to comment #5)
> Steps recorded in those trace-/log-files:
> 
> On the client:
> 
> 1) create file 'a.txt' on the share
> 2) hardlink 'a.txt' to 'a.link.txt'
> 3) open 'a.txt' with FILE_SHARE_DELETE share mode
> 4) delete 'a.link.txt'
> 5) close 'a.txt'
> 
> After that, file 'a.txt' was missing (!) and file 'a.link.txt' was still there
> (which is not expected).
> 
> Client: Windows 7 Ultimate x64
> Server: Samba 3.5.6 (compiled from source) on Ubuntu Linux 10.10
> 

If I leave away steps 3 and 5, then 'a.link.txt' is gone and 'a.txt' preserved (fine). So, steps 3 and 5 are required to provoke the bad outcome.
Comment 7 Volker Lendecke 2010-12-22 15:28:05 UTC
Yep, I can see how that happens. Both opens point to the locking.tdb entry. Step 4 is also implemented with a open/del-on-close/close triple which is not effective, as the other handle on the same inode is still open.

This one is tough given the current Samba architecture.

Thanks for the very to-the-point log and sniff.

Not sure how to handle it though. Jeremy?

Volker
Comment 8 Jeremy Allison 2010-12-22 18:56:08 UTC
Ok, I have an idea as to how we might handle this.

We can reasonably easily fix this specific case. Inside close.c when DELETE_ON_CLOSE is set we check if the pathname associated with the file being closed is the same as the one stored along with the share mode entry, and if not we delete it directly (similar to what we do with posix_unlink).

The problem is we only store one sharename/path pair per open share mode entry, and there could be multiple opens onto multiple symlinks, some of which have delete on close set, and some of which do not.

In order to fix that we'll need to separate out the stored pathname, delete on close flag, and credentials used to set it so each share mode entry can have their own versions.

We can store the delete on close flag as an unused bit in the uint16 flags field inside struct share_mode_entry.

As for sharename/pathname pair - can we store it as a 64-bit hash as another element in the struct share_mode_entry array ? Remember, we don't need the full string to call unlink as we can guarantee that the last closer of the fsp will have the correct sharename/pathname pair if the 64-bit hash matches...

Thoughts ? I think I can code up a version of this for v3-6-test over the holidays for you to look at ?

Jeremy.

Comment 9 Jeremy Allison 2010-12-22 18:57:12 UTC
Arg. In the comment above "onto multiple symlinks" should be "onto multiple hard or symbolic links" of course :-).

Jeremy.