Bug 11467 - Handling of 0 byte resource fork stream
Summary: Handling of 0 byte resource fork stream
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-25 13:43 UTC by Ralph Böhme
Modified: 2015-09-06 17:44 UTC (History)
3 users (show)

See Also:


Attachments
Patch for master (18.58 KB, patch)
2015-08-25 14:02 UTC, Ralph Böhme
no flags Details
Patch for master (21.49 KB, patch)
2015-08-25 15:10 UTC, Ralph Böhme
no flags Details
Patch for master (21.33 KB, patch)
2015-08-31 06:44 UTC, Ralph Böhme
jra: review+
Details
git-am fix that went into master. Applies cleanly to 4.3.next. (21.82 KB, patch)
2015-09-02 17:38 UTC, Jeremy Allison
slow: review+
Details
Patch for 4.2 cherry-picked from 4.3 (21.75 KB, patch)
2015-09-02 20:31 UTC, Ralph Böhme
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2015-08-25 13:43:03 UTC
The resource fork stream is different!

Opening the resource fork stream with O_CREAT mustn't create a visible node in the filesystem, only create a file handle suitable for writing. As long as the creator didn't write into the stream, other openers without O_CREAT MUST get an ENOENT error! 

This is way OS X SMB server implements it.

Similarly, truncating a resource fork stream to 0 bytes MUST remove the filesystem endpoint such that subsequent openers without O_CREAT again get an ENOENT error.
Comment 1 Ralph Böhme 2015-08-25 14:02:49 UTC
Created attachment 11366 [details]
Patch for master
Comment 2 Ralph Böhme 2015-08-25 15:10:45 UTC
Created attachment 11367 [details]
Patch for master

Patchset with an additional patch to unlink ._ files in certain situations.
Comment 3 Jeremy Allison 2015-08-25 23:59:05 UTC
Comment on attachment 11367 [details]
Patch for master

Ralph - in this hunk inside fruit_create_file():

+		fsp->aapl_copyfile_supported = true;
+	}
+
+	/*
+	 * If this is a plain open for existing files, opening an 0
+	 * byte size resource fork MUST fail with
+	 * NT_STATUS_OBJECT_NAME_NOT_FOUND.
+	 *
+	 * Cf the vfs_fruit torture tests in test_rfork_create().
+	 */
+	if (is_afpresource_stream(fsp->fsp_name) &&
+	    create_disposition == FILE_OPEN)
+	{
+		int rc;
+
+		rc = SMB_VFS_STAT(handle->conn, fsp->fsp_name);
+		if (rc != 0) {
+			DBG_ERR("stat %s failed\n", fsp_str_dbg(fsp));
+			goto fail;
+		}
+
+		if (fsp->fsp_name->st.st_ex_size == 0) {
+			status = NT_STATUS_OBJECT_NAME_NOT_FOUND;
+			goto fail;
+		}
 	}

I don't think the :

+		int rc;
+
+		rc = SMB_VFS_STAT(handle->conn, fsp->fsp_name);
+		if (rc != 0) {
+			DBG_ERR("stat %s failed\n", fsp_str_dbg(fsp));
+			goto fail;
+		}
+

part should be needed. By convention, when a file exists on, on return from SMB_VFS_NEXT_CREATE_FILE() the stat struct inside fsp->fsp_name should have already been filled with the valid info from a SMB_VFS_STAT call. Can you test
this code to see if it still passes tests with that part removed ? If not, we need to look at why the underlying SMB_VFS_NEXT_CREATE_FILE() isn't filling in the fsp->fsp_name stat struct info (I'm pretty sure it should, as this is used for the dev,ino pair that are used to uniquely identify the open file).

Jeremy.
Comment 4 Ralph Böhme 2015-08-26 15:35:16 UTC
(In reply to Jeremy Allison from comment #3)
Yes, that still works, thanks! I'll attach an updated patchset later on.
Comment 5 Ralph Böhme 2015-08-31 06:44:38 UTC
Created attachment 11383 [details]
Patch for master

Updated patch without the unneeded stat.
Comment 6 Jeremy Allison 2015-09-01 23:52:35 UTC
Comment on attachment 11383 [details]
Patch for master

Pushed to master with some sight changes. I'll re-upload once it goes into master.
Comment 7 Jeremy Allison 2015-09-02 17:38:45 UTC
Created attachment 11392 [details]
git-am fix that went into master. Applies cleanly to 4.3.next.
Comment 8 Ralph Böhme 2015-09-02 20:31:07 UTC
Created attachment 11394 [details]
Patch for 4.2 cherry-picked from 4.3

I'd like to have this in 4.2 as well. Needed as minor change using DEBUG(1,("")) instead of DBG_WARNING().
Comment 9 Jeremy Allison 2015-09-03 18:12:27 UTC
Comment on attachment 11394 [details]
Patch for 4.2 cherry-picked from 4.3

LGTM.
Comment 10 Jeremy Allison 2015-09-03 18:12:50 UTC
Re-assigning to Karolin for inclusion in 4.3.0, 4.2.next.
Comment 11 Karolin Seeger 2015-09-04 10:40:14 UTC
(In reply to Jeremy Allison from comment #10)
Pushed to autobuild-v4-[2|3]-test.
Comment 12 Karolin Seeger 2015-09-06 17:44:51 UTC
(In reply to Karolin Seeger from comment #11)
Pushed to both branches.
Closing out bug report.

Thanks!