[PATCH] Fix bug #3124 - xcopy /d with samba shares works not as aspected
Jeremy Allison
jra at samba.org
Wed May 21 13:26:08 MDT 2014
The reporter (Andrew Klosterman) revived this old
bug, and did an amazing analysis of the problem
that nailed it completely.
In short, the SMB2 create code path doesn't
correctly return rounded timestamps from the
filesystem whereas all other paths through
the server (SMB1 and SMB2) do so.
I realized this is because the SMB2 server
code doesn't call the wrapper function
put_long_date_timespec() that automatically
takes care of this.
2 patches are attched, #1 fixes the
existing SMB2 close code to use struct timespec
internally, not directly use NTTIME, and
to call put_long_date_timespec() instead of
hand-marshalling the timestamps and doing
the rounding itself. This code was already
doing the right thing (rounding) but the
problem was by doing it by hand and not
going through the API used everywhere else
it's easy to forget to do the rounding, and
make mistakes :-).
Patch #2 fixes the SMB2 create code to
do the same (use struct timespec internally,
not directly use NTTIME) and call put_long_date_timespec()
instead of hand-marshalling the timestamps.
This was the code path that had forgotten
to do the rounding. Oops :-).
The simplest fix would have been to
add the round_timespec() calls to the
create codepath, but that then leaves
us with a server that uses one API
to write out file times for all the SMB2
functions called through the SMB2_GETINFO
code paths, and a *different* (although
functionaly equivalent) set of APIs
being used in the SMB2_CLOSE and SMB2_CREATE
code paths. That disturbed me :-).
With these two patches all the server
code that deals with file timestamps
uses struct timespec internally, and
only does the conversion to NTTIME at
the last marshalling step. Should lead
to fewer bugs in future IMHO :-).
Please review and push if happy !
These patches are for mater, patches for
4.1.x and 4.0.x are attached to the bug:
https://bugzilla.samba.org/show_bug.cgi?id=3124
Jeremy.
-------------- next part --------------
From 46e4fd8391ff19d182fcdc4fd0b7df4388c46ab0 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 21 May 2014 11:31:44 -0700
Subject: [PATCH 1/2] s3: smb2: Move from using SBVAL to put NTTIMEs on the
wire to put_long_date_timespec.
put_long_date_timespec() correctly calls round_timespec()
on the time parameters, and is the correct function to
use when writing *any* file-based NTTIME on the wire.
The smb2_close() code being modified already did this by
hand, and so this doesn't change any of the functionality, only
makes the SMB2 code match all of the other server
code in Samba. Move from using NTTIME variables internally
in the server to struct timespec variables, which is
what all the other server code uses. Only map to
NTTIME as the last step of marshalling the output
data.
Not following the put_long_date_timespec()
convention in the SMB2 create code caused the
round_timespec() step to have been missed in
that code - thus bug:
Bug 3124 - xcopy /d with samba shares works not as aspected
https://bugzilla.samba.org/show_bug.cgi?id=3124
which is a regression from a long-ago bug with
SMB1.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_close.c | 127 +++++++++++++++++++++-------------------------
1 file changed, 57 insertions(+), 70 deletions(-)
diff --git a/source3/smbd/smb2_close.c b/source3/smbd/smb2_close.c
index 3712ffe..5334e1d 100644
--- a/source3/smbd/smb2_close.c
+++ b/source3/smbd/smb2_close.c
@@ -32,10 +32,10 @@ static struct tevent_req *smbd_smb2_close_send(TALLOC_CTX *mem_ctx,
uint16_t in_flags);
static NTSTATUS smbd_smb2_close_recv(struct tevent_req *req,
uint16_t *out_flags,
- NTTIME *out_creation_time,
- NTTIME *out_last_access_time,
- NTTIME *out_last_write_time,
- NTTIME *out_change_time,
+ struct timespec *out_creation_ts,
+ struct timespec *out_last_access_ts,
+ struct timespec *out_last_write_ts,
+ struct timespec *out_change_ts,
uint64_t *out_allocation_size,
uint64_t *out_end_of_file,
uint32_t *out_file_attributes);
@@ -84,10 +84,11 @@ static void smbd_smb2_request_close_done(struct tevent_req *subreq)
struct smbd_smb2_request);
DATA_BLOB outbody;
uint16_t out_flags = 0;
- NTTIME out_creation_time = 0;
- NTTIME out_last_access_time = 0;
- NTTIME out_last_write_time = 0;
- NTTIME out_change_time = 0;
+ connection_struct *conn = req->tcon->compat;
+ struct timespec out_creation_ts = { 0, };
+ struct timespec out_last_access_ts = { 0, };
+ struct timespec out_last_write_ts = { 0, };
+ struct timespec out_change_ts = { 0, };
uint64_t out_allocation_size = 0;
uint64_t out_end_of_file = 0;
uint32_t out_file_attributes = 0;
@@ -96,10 +97,10 @@ static void smbd_smb2_request_close_done(struct tevent_req *subreq)
status = smbd_smb2_close_recv(subreq,
&out_flags,
- &out_creation_time,
- &out_last_access_time,
- &out_last_write_time,
- &out_change_time,
+ &out_creation_ts,
+ &out_last_access_ts,
+ &out_last_write_ts,
+ &out_change_ts,
&out_allocation_size,
&out_end_of_file,
&out_file_attributes);
@@ -128,10 +129,14 @@ static void smbd_smb2_request_close_done(struct tevent_req *subreq)
SSVAL(outbody.data, 0x00, 0x3C); /* struct size */
SSVAL(outbody.data, 0x02, out_flags);
SIVAL(outbody.data, 0x04, 0); /* reserved */
- SBVAL(outbody.data, 0x08, out_creation_time);
- SBVAL(outbody.data, 0x10, out_last_access_time);
- SBVAL(outbody.data, 0x18, out_last_write_time);
- SBVAL(outbody.data, 0x20, out_change_time);
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x08, out_creation_ts);
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x10, out_last_access_ts);
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x18, out_last_write_ts);
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x20, out_change_ts);
SBVAL(outbody.data, 0x28, out_allocation_size);
SBVAL(outbody.data, 0x30, out_end_of_file);
SIVAL(outbody.data, 0x38, out_file_attributes);
@@ -148,10 +153,10 @@ static NTSTATUS smbd_smb2_close(struct smbd_smb2_request *req,
struct files_struct *fsp,
uint16_t in_flags,
uint16_t *out_flags,
- NTTIME *out_creation_time,
- NTTIME *out_last_access_time,
- NTTIME *out_last_write_time,
- NTTIME *out_change_time,
+ struct timespec *out_creation_ts,
+ struct timespec *out_last_access_ts,
+ struct timespec *out_last_write_ts,
+ struct timespec *out_change_ts,
uint64_t *out_allocation_size,
uint64_t *out_end_of_file,
uint32_t *out_file_attributes)
@@ -160,23 +165,18 @@ static NTSTATUS smbd_smb2_close(struct smbd_smb2_request *req,
struct smb_request *smbreq;
connection_struct *conn = req->tcon->compat;
struct smb_filename *smb_fname = NULL;
- struct timespec mdate_ts, adate_ts, cdate_ts, create_date_ts;
uint64_t allocation_size = 0;
uint64_t file_size = 0;
uint32_t dos_attrs = 0;
uint16_t flags = 0;
bool posix_open = false;
- ZERO_STRUCT(create_date_ts);
- ZERO_STRUCT(adate_ts);
- ZERO_STRUCT(mdate_ts);
- ZERO_STRUCT(cdate_ts);
+ ZERO_STRUCTP(out_creation_ts);
+ ZERO_STRUCTP(out_last_access_ts);
+ ZERO_STRUCTP(out_last_write_ts);
+ ZERO_STRUCTP(out_change_ts);
*out_flags = 0;
- *out_creation_time = 0;
- *out_last_access_time = 0;
- *out_last_write_time = 0;
- *out_change_time = 0;
*out_allocation_size = 0;
*out_end_of_file = 0;
*out_file_attributes = 0;
@@ -212,16 +212,16 @@ static NTSTATUS smbd_smb2_close(struct smbd_smb2_request *req,
if (ret == 0) {
flags = SMB2_CLOSE_FLAGS_FULL_INFORMATION;
dos_attrs = dos_mode(conn, smb_fname);
- mdate_ts = smb_fname->st.st_ex_mtime;
- adate_ts = smb_fname->st.st_ex_atime;
- create_date_ts = get_create_timespec(conn, NULL, smb_fname);
- cdate_ts = get_change_timespec(conn, NULL, smb_fname);
+ *out_last_write_ts = smb_fname->st.st_ex_mtime;
+ *out_last_access_ts = smb_fname->st.st_ex_atime;
+ *out_creation_ts = get_create_timespec(conn, NULL, smb_fname);
+ *out_change_ts = get_change_timespec(conn, NULL, smb_fname);
if (lp_dos_filetime_resolution(SNUM(conn))) {
- dos_filetime_timespec(&create_date_ts);
- dos_filetime_timespec(&mdate_ts);
- dos_filetime_timespec(&adate_ts);
- dos_filetime_timespec(&cdate_ts);
+ dos_filetime_timespec(out_creation_ts);
+ dos_filetime_timespec(out_last_write_ts);
+ dos_filetime_timespec(out_last_access_ts);
+ dos_filetime_timespec(out_change_ts);
}
if (!(dos_attrs & FILE_ATTRIBUTE_DIRECTORY)) {
file_size = get_file_size_stat(&smb_fname->st);
@@ -232,19 +232,6 @@ static NTSTATUS smbd_smb2_close(struct smbd_smb2_request *req,
}
*out_flags = flags;
-
- round_timespec(conn->ts_res, &create_date_ts);
- unix_timespec_to_nt_time(out_creation_time, create_date_ts);
-
- round_timespec(conn->ts_res, &adate_ts);
- unix_timespec_to_nt_time(out_last_access_time, adate_ts);
-
- round_timespec(conn->ts_res, &mdate_ts);
- unix_timespec_to_nt_time(out_last_write_time, mdate_ts);
-
- round_timespec(conn->ts_res, &cdate_ts);
- unix_timespec_to_nt_time(out_change_time, cdate_ts);
-
*out_allocation_size = allocation_size;
*out_end_of_file = file_size;
*out_file_attributes = dos_attrs;
@@ -257,10 +244,10 @@ struct smbd_smb2_close_state {
struct files_struct *in_fsp;
uint16_t in_flags;
uint16_t out_flags;
- NTTIME out_creation_time;
- NTTIME out_last_access_time;
- NTTIME out_last_write_time;
- NTTIME out_change_time;
+ struct timespec out_creation_ts;
+ struct timespec out_last_access_ts;
+ struct timespec out_last_write_ts;
+ struct timespec out_change_ts;
uint64_t out_allocation_size;
uint64_t out_end_of_file;
uint32_t out_file_attributes;
@@ -302,10 +289,10 @@ static struct tevent_req *smbd_smb2_close_send(TALLOC_CTX *mem_ctx,
state->in_fsp,
state->in_flags,
&state->out_flags,
- &state->out_creation_time,
- &state->out_last_access_time,
- &state->out_last_write_time,
- &state->out_change_time,
+ &state->out_creation_ts,
+ &state->out_last_access_ts,
+ &state->out_last_write_ts,
+ &state->out_change_ts,
&state->out_allocation_size,
&state->out_end_of_file,
&state->out_file_attributes);
@@ -340,10 +327,10 @@ static void smbd_smb2_close_do(struct tevent_req *subreq)
state->in_fsp,
state->in_flags,
&state->out_flags,
- &state->out_creation_time,
- &state->out_last_access_time,
- &state->out_last_write_time,
- &state->out_change_time,
+ &state->out_creation_ts,
+ &state->out_last_access_ts,
+ &state->out_last_write_ts,
+ &state->out_change_ts,
&state->out_allocation_size,
&state->out_end_of_file,
&state->out_file_attributes);
@@ -355,10 +342,10 @@ static void smbd_smb2_close_do(struct tevent_req *subreq)
static NTSTATUS smbd_smb2_close_recv(struct tevent_req *req,
uint16_t *out_flags,
- NTTIME *out_creation_time,
- NTTIME *out_last_access_time,
- NTTIME *out_last_write_time,
- NTTIME *out_change_time,
+ struct timespec *out_creation_ts,
+ struct timespec *out_last_access_ts,
+ struct timespec *out_last_write_ts,
+ struct timespec *out_change_ts,
uint64_t *out_allocation_size,
uint64_t *out_end_of_file,
uint32_t *out_file_attributes)
@@ -374,10 +361,10 @@ static NTSTATUS smbd_smb2_close_recv(struct tevent_req *req,
}
*out_flags = state->out_flags;
- *out_creation_time = state->out_creation_time;
- *out_last_access_time = state->out_last_access_time;
- *out_last_write_time = state->out_last_write_time;
- *out_change_time = state->out_change_time;
+ *out_creation_ts = state->out_creation_ts;
+ *out_last_access_ts = state->out_last_access_ts;
+ *out_last_write_ts = state->out_last_write_ts;
+ *out_change_ts = state->out_change_ts;
*out_allocation_size = state->out_allocation_size;
*out_end_of_file = state->out_end_of_file;
*out_file_attributes = state->out_file_attributes;
--
1.9.1.423.g4596e3a
From 63ac75f15c4e1cb1c7a14b7cfe514728f324d4b3 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 21 May 2014 11:57:16 -0700
Subject: [PATCH 2/2] s3: smb2: Move from using SBVAL to put NTTIMEs on the
wire to put_long_date_timespec.
put_long_date_timespec() correctly calls round_timespec()
on the time parameters, and is the correct function to
use when writing *any* file-based NTTIME on the wire.
Move from using NTTIME variables internally
in the server to struct timespec variables, which is
what all the other server code uses. Only map to
NTTIME as the last step of marshalling the output
data.
The previous SMB2 create code missed the round_timespec()
call before marshalling.
Bug 3124 - xcopy /d with samba shares works not as aspected
https://bugzilla.samba.org/show_bug.cgi?id=3124
which is a regression from a long-ago bug with
SMB1.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_create.c | 85 +++++++++++++++++++++++-----------------------
1 file changed, 43 insertions(+), 42 deletions(-)
diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 52ed171..4e82e2c 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -80,10 +80,10 @@ static NTSTATUS smbd_smb2_create_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
uint8_t *out_oplock_level,
uint32_t *out_create_action,
- NTTIME *out_creation_time,
- NTTIME *out_last_access_time,
- NTTIME *out_last_write_time,
- NTTIME *out_change_time,
+ struct timespec *out_creation_ts,
+ struct timespec *out_last_access_ts,
+ struct timespec *out_last_write_ts,
+ struct timespec *out_change_ts,
uint64_t *out_allocation_size,
uint64_t *out_end_of_file,
uint32_t *out_file_attributes,
@@ -264,10 +264,11 @@ static void smbd_smb2_request_create_done(struct tevent_req *tsubreq)
DATA_BLOB outdyn;
uint8_t out_oplock_level = 0;
uint32_t out_create_action = 0;
- NTTIME out_creation_time = 0;
- NTTIME out_last_access_time = 0;
- NTTIME out_last_write_time = 0;
- NTTIME out_change_time = 0;
+ connection_struct *conn = smb2req->tcon->compat;
+ struct timespec out_creation_ts = { 0, };
+ struct timespec out_last_access_ts = { 0, };
+ struct timespec out_last_write_ts = { 0, };
+ struct timespec out_change_ts = { 0, };
uint64_t out_allocation_size = 0;
uint64_t out_end_of_file = 0;
uint32_t out_file_attributes = 0;
@@ -283,10 +284,10 @@ static void smbd_smb2_request_create_done(struct tevent_req *tsubreq)
smb2req,
&out_oplock_level,
&out_create_action,
- &out_creation_time,
- &out_last_access_time,
- &out_last_write_time,
- &out_change_time,
+ &out_creation_ts,
+ &out_last_access_ts,
+ &out_last_write_ts,
+ &out_change_ts,
&out_allocation_size,
&out_end_of_file,
&out_file_attributes,
@@ -335,14 +336,18 @@ static void smbd_smb2_request_create_done(struct tevent_req *tsubreq)
SCVAL(outbody.data, 0x03, 0); /* reserved */
SIVAL(outbody.data, 0x04,
out_create_action); /* create action */
- SBVAL(outbody.data, 0x08,
- out_creation_time); /* creation time */
- SBVAL(outbody.data, 0x10,
- out_last_access_time); /* last access time */
- SBVAL(outbody.data, 0x18,
- out_last_write_time); /* last write time */
- SBVAL(outbody.data, 0x20,
- out_change_time); /* change time */
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x08,
+ out_creation_ts); /* creation time */
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x10,
+ out_last_access_ts); /* last access time */
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x18,
+ out_last_write_ts); /* last write time */
+ put_long_date_timespec(conn->ts_res,
+ (char *)outbody.data + 0x20,
+ out_change_ts); /* change time */
SBVAL(outbody.data, 0x28,
out_allocation_size); /* allocation size */
SBVAL(outbody.data, 0x30,
@@ -380,10 +385,10 @@ struct smbd_smb2_create_state {
DATA_BLOB private_data;
uint8_t out_oplock_level;
uint32_t out_create_action;
- NTTIME out_creation_time;
- NTTIME out_last_access_time;
- NTTIME out_last_write_time;
- NTTIME out_change_time;
+ struct timespec out_creation_ts;
+ struct timespec out_last_access_ts;
+ struct timespec out_last_write_ts;
+ struct timespec out_change_ts;
uint64_t out_allocation_size;
uint64_t out_end_of_file;
uint32_t out_file_attributes;
@@ -1065,16 +1070,12 @@ static struct tevent_req *smbd_smb2_create_send(TALLOC_CTX *mem_ctx,
state->out_file_attributes = dos_mode(result->conn,
result->fsp_name);
- unix_timespec_to_nt_time(&state->out_creation_time,
- get_create_timespec(smb1req->conn, result,
- result->fsp_name));
- unix_timespec_to_nt_time(&state->out_last_access_time,
- result->fsp_name->st.st_ex_atime);
- unix_timespec_to_nt_time(&state->out_last_write_time,
- result->fsp_name->st.st_ex_mtime);
- unix_timespec_to_nt_time(&state->out_change_time,
- get_change_timespec(smb1req->conn, result,
- result->fsp_name));
+ state->out_creation_ts = get_create_timespec(smb1req->conn,
+ result, result->fsp_name);
+ state->out_last_access_ts = result->fsp_name->st.st_ex_atime;
+ state->out_last_write_ts = result->fsp_name->st.st_ex_mtime;
+ state->out_change_ts = get_change_timespec(smb1req->conn,
+ result, result->fsp_name);
state->out_allocation_size =
SMB_VFS_GET_ALLOC_SIZE(smb1req->conn, result,
&(result->fsp_name->st));
@@ -1097,10 +1098,10 @@ static NTSTATUS smbd_smb2_create_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
uint8_t *out_oplock_level,
uint32_t *out_create_action,
- NTTIME *out_creation_time,
- NTTIME *out_last_access_time,
- NTTIME *out_last_write_time,
- NTTIME *out_change_time,
+ struct timespec *out_creation_ts,
+ struct timespec *out_last_access_ts,
+ struct timespec *out_last_write_ts,
+ struct timespec *out_change_ts,
uint64_t *out_allocation_size,
uint64_t *out_end_of_file,
uint32_t *out_file_attributes,
@@ -1119,10 +1120,10 @@ static NTSTATUS smbd_smb2_create_recv(struct tevent_req *req,
*out_oplock_level = state->out_oplock_level;
*out_create_action = state->out_create_action;
- *out_creation_time = state->out_creation_time;
- *out_last_access_time = state->out_last_access_time;
- *out_last_write_time = state->out_last_write_time;
- *out_change_time = state->out_change_time;
+ *out_creation_ts = state->out_creation_ts;
+ *out_last_access_ts = state->out_last_access_ts;
+ *out_last_write_ts = state->out_last_write_ts;
+ *out_change_ts = state->out_change_ts;
*out_allocation_size = state->out_allocation_size;
*out_end_of_file = state->out_end_of_file;
*out_file_attributes = state->out_file_attributes;
--
1.9.1.423.g4596e3a
More information about the samba-technical
mailing list