Bug 9130 - Certain xattrs cause Windows error 0x800700FF
Certain xattrs cause Windows error 0x800700FF
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: File services
unspecified
All All
: P1 regression
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-30 22:31 UTC by Justin Maggard
Modified: 2013-04-11 07:39 UTC (History)
1 user (show)

See Also:


Attachments
Patch to skip Solaris internal xattrs (375 bytes, patch)
2012-08-30 22:53 UTC, Justin Maggard
no flags Details
Wireshark trace of error (27.33 KB, application/octet-stream)
2013-03-20 23:36 UTC, Justin Maggard
no flags Details
good capture (11.86 KB, application/octet-stream)
2013-03-21 00:28 UTC, Justin Maggard
no flags Details
git-am fix for 3.6.next. (4.30 KB, patch)
2013-03-27 00:05 UTC, Jeremy Allison
no flags Details
One more minor fix on top. (5.73 KB, patch)
2013-03-27 00:08 UTC, Jeremy Allison
no flags Details
git-am fix for master and 4.0.next. (11.77 KB, patch)
2013-03-27 00:11 UTC, Jeremy Allison
no flags Details
bad capture from solaris (8.64 KB, application/vnd.tcpdump.pcap)
2013-03-27 02:44 UTC, Justin Maggard
no flags Details
git-am fix for 3.6.x (6.76 KB, patch)
2013-03-27 18:57 UTC, Jeremy Allison
ddiss: review+
Details
git-am fix for master and 4.0.x (12.80 KB, patch)
2013-03-27 19:03 UTC, Jeremy Allison
no flags Details
Updated 4.0 patch (14.31 KB, patch)
2013-03-28 18:01 UTC, Jeremy Allison
no flags Details
git-am fix for 4.0.x. (14.37 KB, patch)
2013-03-28 21:26 UTC, Jeremy Allison
no flags Details
Latest git-am fix for 4.0 containing uninitialized variables change. (15.38 KB, patch)
2013-03-29 17:10 UTC, Jeremy Allison
no flags Details
git-am fix for 4.0.x that went into master. (16.34 KB, patch)
2013-04-03 16:25 UTC, Jeremy Allison
ddiss: review+
Details
Updated git-am fix for 4.0.x (16.37 KB, patch)
2013-04-08 03:51 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Maggard 2012-08-30 22:31:01 UTC
Attempting to copy certain files from a Samba 3.6 share to a local Windows 7 PC fails an error popup stating:

Error 0x800700FF: The extended attributes are inconsistent.

After experimenting with several options, I have concluded that (1) this only happens with max protocol set to SMB2, and (2) it only happens with certain xattrs.  It seems to have something to do with the characters in the xattr name.  I found that if there is a hyphen or underscore within the name, this error will occur.  So, 'ab' will work, but 'a-b' or 'a_b' will fail.
Comment 1 Justin Maggard 2012-08-30 22:53:30 UTC
Created attachment 7855 [details]
Patch to skip Solaris internal xattrs

I ran across this issue on a Solaris system due to some internal xattrs, and the attached patch works around my specific issue.  But it is also reproducible on Linux if I manually give a file an xattr with a dash or hyphen using setfattr.
Comment 2 Jeremy Allison 2012-09-10 23:56:15 UTC
Ok, so here's the strange thing....

I just wrote a torture test that creates a file over SMB2 with an EA name of "hyphen-name" and it works fine.

So it's not just the name that matters.

Can you upload a capture of the experiment you did to get the error "The extended attributes are inconsistent" message ?

I need to be able to reproduce this with a torture test before I know I've correctly fixed it. As mentioned above, it's not just the name that matters here.

Jeremy.
Comment 3 Jeremy Allison 2012-09-10 23:57:40 UTC
Sorry, forgot to mention - I was creating against a Win7 SMB2 server (just for completeness).
Comment 4 Justin Maggard 2013-03-20 23:36:57 UTC
Created attachment 8668 [details]
Wireshark trace of error

root@justin-rn314:/data/Backup# getfattr -d New\ Rich\ Text\ Document.rtf
# file: New Rich Text Document.rtf
user.DOSATTRIB=0sMHgyMAAAAwADAAAAEQAAACAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAH6oBhq2Jc4BAAAAAAAAAAA=
user.SAMBA_PAI=0sAgSABwAAAAABYwAAAAAAYwAAAAAAYwAAAAABYwAAAAABYgAAAAAAYgAAAAAC/////w==
user.SUNW_ro="abc"
user.a-b
user.a_b
Comment 5 Justin Maggard 2013-03-21 00:28:22 UTC
Created attachment 8669 [details]
good capture

Changed a_b EA to name a_bb.
(Size also +200, but that doesn't matter).

Jeremy (working as Justin).
Comment 6 Jeremy Allison 2013-03-26 19:19:50 UTC
I understand what is going on here. Patch to follow.
Comment 7 Jeremy Allison 2013-03-27 00:05:09 UTC
Created attachment 8685 [details]
git-am fix for 3.6.next.

Justin, can you confirm this fixes the problem for you and I'll get a 2nd Team member review.

Thanks !

Jeremy.
Comment 8 Jeremy Allison 2013-03-27 00:08:09 UTC
Created attachment 8686 [details]
One more minor fix on top.

Justin, please check it fixes the problem.

Thanks,

Jeremy.
Comment 9 Jeremy Allison 2013-03-27 00:11:33 UTC
Created attachment 8687 [details]
git-am fix for master and 4.0.next.

Justin, please check and confirm it fixes the customer issue.

Cheers,

Jeremy.
Comment 10 Justin Maggard 2013-03-27 02:44:24 UTC
Created attachment 8688 [details]
bad capture from solaris

I tried the 3.6 patch against my Solaris platform (after removing my skip Solaris ACL patch), and it didn't solve the issue.  Here's my xattr list.  From a Win7 client, all I did was create a new file on the share, and then try to copy it to my desktop.

root@justin-res:/data/backup# runat "New Text Document.txt" ls -l -la
total 4
drwxrwxrwt  4 guest guest   5 Mar 26 19:29 .
-rwxrwxrwx+ 1 guest guest   0 Mar 26 19:29 ..
-r--r--r--  1 root  root  156 Mar 26 19:29 SUNWattr_ro
-rw-r--r--  1 root  root  472 Mar 26 19:29 SUNWattr_rw
-rw-rw----  1 guest guest  56 Mar 26 19:29 user.DOSATTRIB
Comment 11 Jeremy Allison 2013-03-27 15:44:40 UTC
This is a different problem. It's now correctly returning the 2 Solaris EA's of 
SUNWattr_ro and SUNWattr_rw, neither of which are zero length.

Who creates these attributes ? It isn't Samba.

This is a different test case from the one you originally reported. If you test with the original EA's of:

user.SUNW_ro="abc"
user.a-b
user.a_b

as we had in the test case, I think you'll find it works :-).

If we can I'd like to get the original test case verified as fixed, then I can work on this new issue either as an auxiliary case (if it's a related problem) or as a new bug.

SUNWattr_ro is being correctly returned (the ls shows a size of 156, which is correctly returned inside the EA). However SUNWattr_rw show an ls size of 472, but is being returned inside the EA as a length of 216.

Of course the client shouldn't care about any of that :-).

Jeremy.
Comment 12 Jeremy Allison 2013-03-27 17:40:17 UTC
Oh never mind :-). Wireshark is missing reading the upper length bytes of 0x1, which when added to the lower length byte of 0xd8 leaves the correct length (0x1d8 == 472).

So I can't immediately see why Windows is objecting to this EA return.
Comment 13 Justin Maggard 2013-03-27 18:12:53 UTC
You are correct, the original test case does indeed work now. :-)

This new issue does appear to be related to the EA data lengths somehow, because I can reproduce it on Linux by creating EA's with data lengths matching those on the Solaris box.
Comment 14 Jeremy Allison 2013-03-27 18:57:10 UTC
Created attachment 8690 [details]
git-am fix for 3.6.x

Found the problem. From the additional patch..

    The spec lies when it says that NextEntryOffset is the only value
    considered when finding the next EA. We were adding 4 more extra
    pad bytes than needed (i.e. if the next entry already was on a 4
    byte boundary, then we were adding 4 additional pad bytes).

Try with this patchset. Fixes the problem for me.

Jeremy.
Comment 15 Jeremy Allison 2013-03-27 19:03:44 UTC
Created attachment 8691 [details]
git-am fix for master and 4.0.x

Same patchset for master/4.0.x. Once Justin confirms I'll submit to master and then get review on 4.0.x.

Jeremy.
Comment 16 Justin Maggard 2013-03-27 21:54:52 UTC
Great!  That's got it fixed for me on both platforms.
Comment 17 Jeremy Allison 2013-03-28 18:01:30 UTC
Created attachment 8696 [details]
Updated 4.0 patch

Justin, sorry to trouble you but there were a couple of (mostly cosmetic :-) change requests to the 4.0.x patch, so I was wondering if you could re-test with this version just to make sure what goes into 4.0.x works for your product.

Sorry for the trouble,

Jeremy.
Comment 18 Justin Maggard 2013-03-28 19:05:29 UTC
No problem.  I have confirmed that the updated 4.0 patch also works for me.
Comment 19 Jeremy Allison 2013-03-28 19:37:15 UTC
Comment on attachment 8690 [details]
git-am fix for 3.6.x

David, please review the 3.6.next version (minimal patch for 3.6.next only).
Comment 20 Jeremy Allison 2013-03-28 21:26:07 UTC
Created attachment 8699 [details]
git-am fix for 4.0.x.

This is the fix that was pushed to master.

David, please +1 on this and the 3.6.x fix and I'll re-assign to Karolin.

Jeremy.
Comment 21 Jeremy Allison 2013-03-29 17:10:39 UTC
Created attachment 8701 [details]
Latest git-am fix for 4.0 containing uninitialized variables change.
Comment 22 Jeremy Allison 2013-03-29 17:18:18 UTC
Comment on attachment 8690 [details]
git-am fix for 3.6.x

Uninitialized variables change not needed for 3.6.x, get_ea_list_from_file() already returns NULL if no EA list.
Comment 23 Jeremy Allison 2013-04-02 03:01:58 UTC
Marking this one as a regresssion in 4.0.x, as we really do need to get this in for 4.0.next. Patch just needs to go into master (I'm working with David to get this finalized).

Jeremy.
Comment 24 David Disseldorp 2013-04-03 09:38:49 UTC
Comment on attachment 8701 [details]
Latest git-am fix for 4.0 containing uninitialized variables change.

Jeremy, the patch for 4.0 should also carry the selftest/knownfail change in master (7bee3ef68490bb38942d717e03e203d00be32f9f), right?
Comment 25 David Disseldorp 2013-04-03 11:17:52 UTC
Comment on attachment 8690 [details]
git-am fix for 3.6.x

Would have been good to include the same fill_ea_chained_buffer() size calculation change as is in master (d9e7c8219fd8b3d770301a87bc1cd62b07b989ca), but looks okay as is.
Comment 26 Jeremy Allison 2013-04-03 16:10:19 UTC
Comment on attachment 8690 [details]
git-am fix for 3.6.x

I want to make as few changes to 3.6.x as possible. This is the minimal fix (IMHO).

Jeremy.
Comment 27 Jeremy Allison 2013-04-03 16:25:26 UTC
Created attachment 8719 [details]
git-am fix for 4.0.x that went into master.

Here's the fix that went into master (with the added selftest/knownfail change).
Jeremy.
Comment 28 David Disseldorp 2013-04-04 08:58:25 UTC
Comment on attachment 8719 [details]
git-am fix for 4.0.x that went into master.

Looks good.
Comment 29 Jeremy Allison 2013-04-04 15:40:20 UTC
Re-assigning to Karolin for inclusion in 3.6.next and 4.0.next.
Jeremy.
Comment 30 Karolin Seeger 2013-04-07 19:40:26 UTC
Pushed to v3-6-test.
Comment 31 Karolin Seeger 2013-04-07 19:41:32 UTC
Applying the patch set to v4-0-test fails with:

user@host:/data/git/samba/v4-0-test$ git am bug-9130-4.0.patch
Applying: Ensure we can never return an uninitialized EA list.
Applying: Modify fill_ea_chained_buffer() to be able to do size calculation only, no marshalling.
Applying: Change estimate_ea_size() to correctly estimate the EA size over SMB2.
Applying: Fix bug #9130 - Certain xattrs cause Windows error 0x800700FF
Applying: Fix bug #9130 - Certain xattrs cause Windows error 0x800700FF
Applying: Add a test to show that zero-length EA's are never returned over SMB2.
error: patch failed: selftest/knownfail:159
error: selftest/knownfail: patch does not apply
Patch failed at 0006 Add a test to show that zero-length EA's are never returned over SMB2.
Comment 32 Jeremy Allison 2013-04-08 03:40:47 UTC
Hmmm. I remember fixing that issue, but maybe I did it for the patch that got obsoleted by the later fix that went into master.

Can you hold off on 4.0.next until tomorrow morning (Monday Pacific Time) when I'll get to this asap ?

It's just a matter of fixing the surrounding diff context for the selftest/knownfail fix, all the rest of the patch applies cleanly.

Sorry for the problem.

Jeremy.
Comment 33 Jeremy Allison 2013-04-08 03:51:00 UTC
Created attachment 8733 [details]
Updated git-am fix for 4.0.x

This is identical to the fix that David reviewed, with the change of the diff -u context in the selftest/knownfail change.

Applies cleanly to 4.0.next.

Please apply (I decided it was quicker to do it now than wait for Monday morning Pacific Time :-).

Cheers,

Jeremy.
Comment 34 Karolin Seeger 2013-04-08 06:46:56 UTC
(In reply to comment #33)
> Created attachment 8733 [details]
> Updated git-am fix for 4.0.x
> 
> This is identical to the fix that David reviewed, with the change of the diff
> -u context in the selftest/knownfail change.
> 
> Applies cleanly to 4.0.next.
> 
> Please apply (I decided it was quicker to do it now than wait for Monday
> morning Pacific Time :-).
> 
> Cheers,
> 
> Jeremy.

Pushed to autobuild-v4-0-test.
Thanks for the quick reply!
Comment 35 Karolin Seeger 2013-04-11 07:39:59 UTC
Pushed to v4-0-test.
Closing out bug report.

Thanks!