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: ASSIGNED
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: Ralph Böhme
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-16 17:49 UTC by Ralph Böhme
Modified: 2021-07-09 16:45 UTC (History)
5 users (show)

See Also:


Attachments
DO NOT USE - sample patch only (6.98 KB, application/mbox)
2021-07-09 16:44 UTC, Ashok Ramakrishnan
no flags 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,