Bug 14127 - Samba doesn't implement semantics of setting a file's timestamp to -1 and -2
Summary: Samba doesn't implement semantics of setting a file's timestamp to -1 and -2
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-16 17:49 UTC by Ralph Böhme
Modified: 2021-12-30 09:20 UTC (History)
7 users (show)

See Also:


Attachments
DO NOT USE - sample patch only (6.98 KB, patch)
2021-07-09 16:44 UTC, Ashok Ramakrishnan
no flags Details
Patch for 4.14 and 4.15 cherry-picked from master. (14.84 KB, patch)
2021-10-28 19:33 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2019-09-16 17:49:10 UTC

    
Comment 1 Ralph Böhme 2019-09-16 17:53:36 UTC
As found out in discussions with Microsoft engineers at an interop event, when a client requests setting timestamps on a file to -1 or -2 this requests special behaviour from the server on the filehandle.

-1 disable timestamp updates on subsequent IO operations on the file, -2 restores it.

Link to MS doc to follow.
Comment 3 Ashok Ramakrishnan 2021-07-09 16:06:11 UTC
Hi Ralph:
This seems to be causing samba to write an invalid timestamp to the last access field. We noticed this with Revit Autodesk application failing to open a file and the issue started with some recent windows update (not sure of the first release that introduced this, but know that 19043.1052 has the problem). 

Lack of support for handling freezing and unfreezing updates is one thing, but it appears that the lack of support for handling/ignoring the -2 value is more troublesome. My patch to ignore -2, fwiw... i really wish the protocol had a better mechanism to negotiate this support instead of assuming all implementation would support this...or maybe there is and I am not seeing it...

My patch (without the smbtorture changes), fwiw…

diff --git a/lib/util/time.c b/lib/util/time.c
index 0fac5e2..3ec482f 100644
--- a/lib/util/time.c
+++ b/lib/util/time.c
@@ -1129,10 +1129,11 @@ struct timespec nt_time_to_full_timespec(NTTIME nt)
        if (nt == NTTIME_OMIT) {
                return make_omit_timespec();
        }
-       if (nt == NTTIME_FREEZE) {
+       if (nt == NTTIME_FREEZE || nt == NTTIME_THAW) {
                /*
-                * This should be returned as SAMBA_UTIME_FREEZE in the
-                * future.
+                * This should be returned as SAMBA_UTIME_FREEZE or
+                * SAMBA_UTIME_THAW in the future - when support is
+                * added for those operations.
                 */
                return make_omit_timespec();
        }
diff --git a/lib/util/time.h b/lib/util/time.h
index 4a90b40..5d1bc90 100644
--- a/lib/util/time.h
+++ b/lib/util/time.h
@@ -63,6 +63,7 @@
  * implement this yet.
  */
#define NTTIME_FREEZE UINT64_MAX
+#define NTTIME_THAW ((uint64_t) -2)

#define SAMBA_UTIME_NOW UTIME_NOW
#define SAMBA_UTIME_OMIT UTIME_OMIT
Comment 4 Ralph Böhme 2021-07-09 16:18:02 UTC
(In reply to Ashok Ramakrishnan from comment #3)
Correclty ignoring -2 is definitely a step in the right direction to avoid writing strange timestamp values. :)
To avoid regresssion, can you also share torture tests that you mention? Ideally if you could please wrap it all up in a merge request on gitlab, that would help a lot.
Comment 5 Ashok Ramakrishnan 2021-07-09 16:44:30 UTC
Created attachment 16678 [details]
DO NOT USE - sample patch only
Comment 6 Ashok Ramakrishnan 2021-07-09 16:45:21 UTC
Comment on attachment 16678 [details]
DO NOT USE - sample patch only

In my experiments, the torture test is failing some of the operations (probably because limited file system support) even without the patch. And, I have not had the opportunity to dig deeper into those. So, the torture change I have may not be applied as is. Attaching the patch here anyway fwiw. thanks,
Comment 7 Peter Eriksson 2021-09-29 14:46:21 UTC
Just a note for reference:  
This same (or similar) issue seems to be also triggered by Matlab (when compiling stuff) and also Arena Simulation Software (Process Analyzer) when being run from an up to date Windows 10 client (20H2) against Samba 4.15.0 on FreeBSD 12.2 with ZFS filesystems. Using older Windows 10 version (1909) seems to work fine (until it gets patched with updates :-)


Both programs complain about corrupted files.

A more detail look from Windows clients gives that:

Newly written files (by the programs in question) are OK immediately after they are written. 

A short while later (5-15s?) they become unreadable by the software. The unreadability can also be triggered by browsing the directory (from the software).

Looking at the file metadata (from Windows) indicates that two fields in the "File Properties" give wrong data:

"Size on disk" changes from some value to 0.
"Accessed" changes from a valid value to 00:00:00

I'm guessing the -2 causes some stat cache to be corrupted...


Anyway, running Samba 4.15.0 with the patch seems to fix the issue.
Comment 8 Ralph Böhme 2021-09-29 15:08:34 UTC
(In reply to Peter Eriksson from comment #7)
Oh, sorry to hear that! Not sure anyone is currently working on this. It's certainly not a multiweeks effort, but it will easily take a few days to carefully write test and fully excersize the correct Windows behaviour. I wanted to pick this up for a long time, but haven't been able due to resource constraints.
Comment 9 Peter Eriksson 2021-09-29 15:25:13 UTC
No big problem (for us) now that there is a patch that fixes it (I also run home-built Sambas anyway :-)

Btw, another way to "resurrect" the "corrupt" files (temporarily) is to use a PowerShell script to simply set the timestamp to "now", then it works again (for a while). I'm guessing this causes some stat cache in Samba to be updated.

(I've been scratching my head about this issue for a couple of weeks so this was nice to find. I'm guessing I might not be the only one :-)
Comment 10 Ralph Böhme 2021-09-29 15:30:36 UTC
(In reply to Peter Eriksson from comment #9)
Fwiw, there's no statcache in Samba. I know there's something called statcache, but that's just a "path existince" patch.
Comment 11 Jeremy Allison 2021-09-29 16:00:37 UTC
Yeah, there's no stat info cache in Samba, what we call "statcache" is merely a quick "file exists under normalized name" check.

I wasn't aware at *ALL* of "NTTIME_THAW". From:

https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/ns-wdm-_file_basic_information

On NTFS and ReFS systems, time stamp updates on the file handle can be restored by setting the appropriate member(s) to -2.

I didn't know applications used that behavior. As it's application visible we need to bug ms-dochelp to document this.
Comment 12 Ralph Böhme 2021-09-29 16:05:01 UTC
(In reply to Jeremy Allison from comment #11)
What do you mean "visible we need to bug ms-dochelp to document this"? It *is* documented in the page you linked.
Comment 13 Jeremy Allison 2021-09-29 16:41:08 UTC
(In reply to Ralph Böhme from comment #12)

Yes, in a note in a "Windows Driver" document on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying "Beware of the Leopard."

:-). This needs to be somewhere more visible in the protocol documentation stack.
Comment 14 Samba QA Contact 2021-10-28 19:04:03 UTC
This bug was referenced in samba master:

f73aff502cadabb7fe6b94a697f0a2256d1d4aca
d84779302cc54a7b84c05ccc458e04b27fd142f4
e2740e4868f2a49877a86a8666d26226b5657317
5503bde93bddf3634b183e665773399c110251d4
194faa76161a12ae1eae2b471d6f159d97ef75a8
0659069f8292996be475d407b53d161aa3f35554
6ed71ad7e6aa98a34cfde95d7d62c46694d58469
Comment 15 Jeremy Allison 2021-10-28 19:33:18 UTC
Created attachment 16885 [details]
Patch for 4.14 and 4.15 cherry-picked from master.

Cherry-picked from master. Do we want this for 4.14.next also ?
Comment 16 Ralph Böhme 2021-10-29 03:36:00 UTC
Comment on attachment 16885 [details]
Patch for 4.14 and 4.15 cherry-picked from master.

Yes, we need this in 4.14 and 4.15. Patch applies cleanly to 4.14 as well.
Comment 17 Ralph Böhme 2021-10-29 03:36:29 UTC
Reassigning to Jule for inclusion in 4.14 and 4.15.
Comment 18 Jule Anger 2021-11-10 14:22:16 UTC
Pushed to autobuild-v4-{15,14}-test.
Comment 19 Samba QA Contact 2021-11-10 15:55:03 UTC
This bug was referenced in samba v4-14-test:

ac6f4c093b82501a3282c142b4912c501b2c2999
43f873d52ab9e17209595677c4672de19da7c65a
f8fec80020e8aadba8b49a3f0247009534ab7410
38ac4c094749d880ae8b1e25942ebb7b1c182c49
7e1a65ed980afe6d5c987baf98d6f3700bbb9145
4e2c7c66c9699276f3fb1b81c4a6f574068d7141
4a106c2322c5f69f008707701ca54904b51cb297
Comment 20 Samba QA Contact 2021-11-10 17:06:04 UTC
This bug was referenced in samba v4-15-test:

0acbd644fcd2ea48ff674f8ad031069c50706a5e
60adfb19d9d13df0a4127214f0a80d219d85d5f1
0b7c1089d1200fa2de6f2565548e8cc795d6685d
bfb893f5efcf9db1803e8cbc2c87f3b2606f6399
6e42b2a1670c83e1e6dcb695841be1699db21278
204f1488e2c2314195d4988e625629058fe26978
c99eecaf2fb0390fd071c5636cf3bc64ae287b8d
Comment 21 Jule Anger 2021-11-10 18:03:03 UTC
Closing out bug report.

Thanks!
Comment 22 Samba QA Contact 2021-12-08 14:39:32 UTC
This bug was referenced in samba v4-15-stable (Release samba-4.15.3):

0acbd644fcd2ea48ff674f8ad031069c50706a5e
60adfb19d9d13df0a4127214f0a80d219d85d5f1
0b7c1089d1200fa2de6f2565548e8cc795d6685d
bfb893f5efcf9db1803e8cbc2c87f3b2606f6399
6e42b2a1670c83e1e6dcb695841be1699db21278
204f1488e2c2314195d4988e625629058fe26978
c99eecaf2fb0390fd071c5636cf3bc64ae287b8d
Comment 23 Samba QA Contact 2021-12-15 14:53:10 UTC
This bug was referenced in samba v4-14-stable (Release samba-4.14.11):

ac6f4c093b82501a3282c142b4912c501b2c2999
43f873d52ab9e17209595677c4672de19da7c65a
f8fec80020e8aadba8b49a3f0247009534ab7410
38ac4c094749d880ae8b1e25942ebb7b1c182c49
7e1a65ed980afe6d5c987baf98d6f3700bbb9145
4e2c7c66c9699276f3fb1b81c4a6f574068d7141
4a106c2322c5f69f008707701ca54904b51cb297