[Samba] Re: [smbd][PATCH] Writing EAs when overriding files

[Günter Kukkukk]

Am Freitag, 15. Mai 2009 schrieb Marcel Müller:
> Hi,
> 
> I think I found a bug in smbd. When a file is opened in 
> overwritten/truncated the extended attributes are not applied.
> 
> Since the Win32 API does not have a single function that overwrites a 
> file and associates extended attributes in one call, it is very unlikely 
> to cause any harm to windows clients. But OS/2 clients have this API 
> function and the LANMAN2 trans2open request is forwarded to create_file.
> 
> I have no sufficiently detailed documentation of the LANMAN2 protocol, 
> but the OS/2 API docs for DosOpen clearly say:
> 
>    "peaop2 (PEAOP2) - in/out   Extended attributes.
> This parameter is only used to specify extended attributes (EAs) when 
> creating a new file, replacing an existing file, or truncating an 
> existing file. When opening existing files, it should be set to null."
> 
> And usually the OS/2 file system API is nearly a 1:1 image of the 
> LANMAN2 protocol.
> 
> 
> The relating code is in open.c in the function create_file (samba 3.2)
> 
> -	if ((ea_list != NULL) && (info == FILE_WAS_CREATED)) {
> +	if ((ea_list != NULL) && (info != FILE_WAS_OPENED)) {
> 		status = set_ea(conn, fsp, fname, ea_list);
> 		if (!NT_STATUS_IS_OK(status)) {
> 			goto fail;
> 		}
> 	}
> 
> 
> The same applies to samba 3.0. But the related code is located in 
> trans2.c in this case.
> 
> - if (total_data && smb_action == FILE_WAS_CREATED) {
> + if (total_data && smb_action != FILE_WAS_OPENED) {
> 
> 
> Marcel
> 

Hi Marcel,

you're right regarding the IBM document about the peaop2 parm in the
DosOpen() call.

Your proposed patch
-	if ((ea_list != NULL) && (info == FILE_WAS_CREATED)) {
+	if ((ea_list != NULL) && (info != FILE_WAS_OPENED)) {

is _only_ a first step to get it working right.

-----------------
When a file is opened e.g. in the "truncate" mode, windows and os/2 not
only reset the file size to zero - they _also_ remove all _former_
extended attributes!

This is atm not the case on linux (*nix ?). BUG !!!?

Samba could/should work around this behaviour.

Linux doesn't allow to delete all xattrs at once, so
one must iterate over all existings xattr and delete them - and
_then_ the new passed xattr have to be set.

So more coding is needed here ...
If the former xattr are not deleted before the new ones are applied,
we'll see them accumulating ...
-----------------

Most of this was already discussed in 2005:
http://lists.samba.org/archive/samba-technical/2005-November/043793.html
http://lists.samba.org/archive/samba-technical/2005-November/043794.html

Btw- there is some more xattr info on 
http://lists.samba.org/archive/samba-technical/2005-November/
regarding "Is the sky actually falling?....."
----

Jeremy, Volker - as a side note. Is this former/current linux behaviour
really the right *nix approach?

Today the coreutils/fileutils "cp" options have changed a bit:

touch f1 f2 f3
setfattr -n $'user.GK0' -v $'Some text for GK0' f1
setfattr -n $'user.CB0' -v $'Some text for CB0' f2
setfattr -n $'user.MY0' -v $'Some text for MY0' f3
getfattr -d f3

cp --preserve=xattr f1 f3
cp --preserve=xattr f2 f3
getfattr -d f3

Ouch ...

To me, that xattr accumulating behaviour is just _wrong_ - even
a security bug, cause former "file info" is still there...
A dest file copy is a 100% copy of the source - point!
(how does SELinux cope with that ...?)

Ok - that's atm the *nix way...

Cheers, Günter
--

Coreutils related:
http://www.mail-archive.com/bug-coreutils@gnu.org/msg14605.html
http://www.mail-archive.com/bug-coreutils@gnu.org/msg15257.html

Btw - how are windows "alternate data streams" handled in the "truncate"
or "overwrite" case ?