[PATCH] Add pidhigh in the common response header
Jeremy Allison
jra at samba.org
Sun Jun 12 01:02:31 UTC 2016
On Thu, Apr 14, 2016 at 10:03:39PM -0700, Jeremy Allison wrote:
> > in the SMB1 byte range locking.
> >
> > The pid sent and stored in the SMB1 byte range
> > lock requests is limited to 16-bits by definition.
> >
> > This means it doesn't look at pidhigh (only pidlow)
> > when checking for blocking_smblctx conflicts.
> >
> > Actually, even though I think this change is correct,
> > what I should really do is add another varient
> > of the source4/torture/raw/lock.c that sets a >16-bit
> > pid on the initial lock, and see if the MOD(>16-bit)
> > pid context conflicts.
>
> Ah. Just realized that can't be done, as that
> field (pid in the lock context) is only 16-bits.
>
> Essentially this means the use of the pid in
> SMB1 lock context is inherently broken and can't
> conflict correctly as designed. So the additional
> test I need to do is ensure that a Windows server
> reflects highpid on reply in the same way that
> this patch would make us do.
Finally I got around to this :-).
Here is the complete patchset (including regression
test) that makes us behave exactly like a Windows
server w.r.t returning the correct pidlow and pidhigh
on SMB1.
This doesn't affect the lock context code or tests as they
(by protocol design) only look at the lower 16 bits, even
if pidhigh is set. With this patchset we behave exactly
like a Windows server for locking and SMB1 pid replies.
Alexander (or another Team member), please review and
push if happy !
Cheers,
Jeremy.
-------------- next part --------------
>From db6693bca16c33497644bd33bbeb7f4d5b49fedb Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 10 Jun 2016 16:15:22 -0700
Subject: [PATCH 1/4] s4: libcli: Internal SMB1 pid is already stored as and
uses 32-bits. Correct getpid() cast.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source4/libcli/raw/clisession.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source4/libcli/raw/clisession.c b/source4/libcli/raw/clisession.c
index 23d7fd1..0a026c0 100644
--- a/source4/libcli/raw/clisession.c
+++ b/source4/libcli/raw/clisession.c
@@ -52,7 +52,7 @@ struct smbcli_session *smbcli_session_init(struct smbcli_transport *transport,
} else {
session->transport = talloc_reference(session, transport);
}
- session->pid = (uint16_t)getpid();
+ session->pid = (uint32_t)getpid();
session->vuid = UID_FIELD_INVALID;
session->options = options;
--
2.7.4
>From d91a49525dcee8b3bcdaf9a11328d095504ba27c Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 10 Jun 2016 16:51:11 -0700
Subject: [PATCH 2/4] s3: libsmb: Widen the internal client smb1.pid to 32-bits
as is used on the wire and in libcli/smb/smb1*.c
Note: This has *NO* effect on the lock context code, as on the
wire for all SMB1 locking requests, the pid used as the lock
context is already truncated down to 16-bits - the field is only
16-bits wide.
This allows the cli_XXX() calls to correctly set pidlow AND pidhigh
in SMB1 requests put on the wire by the libcli/smb/smb1*.c code.
Note that currently the smbd server doesn't correctly return
pidhigh yet - a fix (and tests) for that will follow.
As pidhigh is not checked in any client code (mid is used
to differentiate different requests) this has no effect
other than a correctness fix.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/include/client.h | 2 +-
source3/libsmb/clientgen.c | 8 ++++----
source3/libsmb/proto.h | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/source3/include/client.h b/source3/include/client.h
index 0024c04..43ec39b 100644
--- a/source3/include/client.h
+++ b/source3/include/client.h
@@ -91,7 +91,7 @@ struct cli_state {
const char *remote_realm;
struct {
- uint16_t pid;
+ uint32_t pid;
uint16_t vc_num;
struct smbXcli_session *session;
struct smbXcli_tcon *tcon;
diff --git a/source3/libsmb/clientgen.c b/source3/libsmb/clientgen.c
index cfb3b16..bf31bb1 100644
--- a/source3/libsmb/clientgen.c
+++ b/source3/libsmb/clientgen.c
@@ -225,7 +225,7 @@ struct cli_state *cli_state_create(TALLOC_CTX *mem_ctx,
goto error;
}
- cli->smb1.pid = (uint16_t)getpid();
+ cli->smb1.pid = (uint32_t)getpid();
cli->smb1.vc_num = cli->smb1.pid;
cli->smb1.tcon = smbXcli_tcon_create(cli);
if (cli->smb1.tcon == NULL) {
@@ -327,14 +327,14 @@ uint16_t cli_state_get_vc_num(struct cli_state *cli)
Set the PID to use for smb messages. Return the old pid.
****************************************************************************/
-uint16_t cli_setpid(struct cli_state *cli, uint16_t pid)
+uint32_t cli_setpid(struct cli_state *cli, uint32_t pid)
{
- uint16_t ret = cli->smb1.pid;
+ uint32_t ret = cli->smb1.pid;
cli->smb1.pid = pid;
return ret;
}
-uint16_t cli_getpid(struct cli_state *cli)
+uint32_t cli_getpid(struct cli_state *cli)
{
return cli->smb1.pid;
}
diff --git a/source3/libsmb/proto.h b/source3/libsmb/proto.h
index 1e358f7..c5e74c9 100644
--- a/source3/libsmb/proto.h
+++ b/source3/libsmb/proto.h
@@ -172,8 +172,8 @@ void cli_nt_pipes_close(struct cli_state *cli);
void cli_shutdown(struct cli_state *cli);
const char *cli_state_remote_realm(struct cli_state *cli);
uint16_t cli_state_get_vc_num(struct cli_state *cli);
-uint16_t cli_setpid(struct cli_state *cli, uint16_t pid);
-uint16_t cli_getpid(struct cli_state *cli);
+uint32_t cli_setpid(struct cli_state *cli, uint32_t pid);
+uint32_t cli_getpid(struct cli_state *cli);
bool cli_state_has_tcon(struct cli_state *cli);
uint16_t cli_state_get_tid(struct cli_state *cli);
uint16_t cli_state_set_tid(struct cli_state *cli, uint16_t tid);
--
2.7.4
>From a212a599cc8267cdc66ceb08f1c3501b9f5ea60e Mon Sep 17 00:00:00 2001
From: Per Forlin <per.forlin at gmail.com>
Date: Fri, 10 Jun 2016 17:00:55 -0700
Subject: [PATCH 3/4] s3: smbd: Correctly reflect back SMB_PIDHIGH to a client.
Torture test to follow.
Signed-off-by: Per Forlin <per.forlin at gmail.com>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/process.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index 34939f0..e3c32f9 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -2077,6 +2077,7 @@ static void construct_reply_common(uint8_t cmd, const uint8_t *inbuf,
SSVAL(outbuf,smb_tid,SVAL(inbuf,smb_tid));
SSVAL(outbuf,smb_pid,SVAL(inbuf,smb_pid));
+ SSVAL(outbuf,smb_pidhigh,SVAL(inbuf,smb_pidhigh));
SSVAL(outbuf,smb_uid,SVAL(inbuf,smb_uid));
SSVAL(outbuf,smb_mid,SVAL(inbuf,smb_mid));
}
--
2.7.4
>From 0b9ea81e2c0d00c945194878b8f8422efd7fad43 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Sat, 11 Jun 2016 17:51:16 -0700
Subject: [PATCH 4/4] s3: torture: Add test that proves Win2k12 correctly
returns pidlow and pidhigh in SMB1 requests.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail | 1 +
source3/selftest/tests.py | 2 +-
source3/torture/torture.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 161 insertions(+), 1 deletion(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 2f2d6bf..be6c70c 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -19,6 +19,7 @@
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-SYMLINK-ACL # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-SYMLINK-EA # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).POSIX-OFD-LOCK # Fails against the s4 ntvfs server
+^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).PIDHIGH # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).NTTRANS-FSCTL # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).SMB2-NEGPROT # Fails against the s4 ntvfs server
^samba3.smbtorture_s3.plain\(ad_dc_ntvfs\).BAD-NBT-SESSION # Fails against the s4 ntvfs server
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index b96df8a..4d66a5d 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -50,7 +50,7 @@ tests = ["FDPASS", "LOCK1", "LOCK2", "LOCK3", "LOCK4", "LOCK5", "LOCK6", "LOCK7"
"DIR", "DIR1", "DIR-CREATETIME", "TCON", "TCONDEV", "RW1", "RW2", "RW3", "LARGE_READX", "RW-SIGNING",
"OPEN", "XCOPY", "RENAME", "DELETE", "DELETE-LN", "WILDDELETE", "PROPERTIES", "W2K",
"TCON2", "IOCTL", "CHKPATH", "FDSESS", "CHAIN1", "CHAIN2",
- "CHAIN3",
+ "CHAIN3", "PIDHIGH",
"GETADDRINFO", "UID-REGRESSION-TEST", "SHORTNAME-TEST",
"CASE-INSENSITIVE-CREATE", "SMB2-BASIC", "NTTRANS-FSCTL", "SMB2-NEGPROT",
"SMB2-SESSION-REAUTH", "SMB2-SESSION-RECONNECT",
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index ea0fc01..0926690 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -8622,6 +8622,164 @@ static bool run_streamerror(int dummy)
return ret;
}
+struct pidtest_state {
+ bool success;
+ uint16_t vwv[1];
+ DATA_BLOB data;
+};
+
+static void pid_echo_done(struct tevent_req *subreq);
+
+static struct tevent_req *pid_echo_send(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct cli_state *cli)
+{
+ struct tevent_req *req, *subreq;
+ struct pidtest_state *state;
+
+ req = tevent_req_create(mem_ctx, &state, struct pidtest_state);
+ if (req == NULL) {
+ return NULL;
+ }
+
+ SSVAL(state->vwv, 0, 1);
+ state->data = data_blob_const("hello", 5);
+
+ subreq = smb1cli_req_send(state,
+ ev,
+ cli->conn,
+ SMBecho,
+ 0, 0, /* *_flags */
+ 0, 0, /* *_flags2 */
+ cli->timeout,
+ 0xDEADBEEF, /* pid */
+ NULL, /* tcon */
+ NULL, /* session */
+ ARRAY_SIZE(state->vwv), state->vwv,
+ state->data.length, state->data.data);
+
+ if (tevent_req_nomem(subreq, req)) {
+ return tevent_req_post(req, ev);
+ }
+ tevent_req_set_callback(subreq, pid_echo_done, req);
+ return req;
+}
+
+static void pid_echo_done(struct tevent_req *subreq)
+{
+ struct tevent_req *req = tevent_req_callback_data(
+ subreq, struct tevent_req);
+ struct pidtest_state *state = tevent_req_data(
+ req, struct pidtest_state);
+ NTSTATUS status;
+ uint32_t num_bytes;
+ uint8_t *bytes = NULL;
+ struct iovec *recv_iov = NULL;
+ uint8_t *phdr = NULL;
+ uint16_t pidlow = 0;
+ uint16_t pidhigh = 0;
+ struct smb1cli_req_expected_response expected[] = {
+ {
+ .status = NT_STATUS_OK,
+ .wct = 1,
+ },
+ };
+
+ status = smb1cli_req_recv(subreq, state,
+ &recv_iov,
+ &phdr,
+ NULL, /* pwct */
+ NULL, /* pvwv */
+ NULL, /* pvwv_offset */
+ &num_bytes,
+ &bytes,
+ NULL, /* pbytes_offset */
+ NULL, /* pinbuf */
+ expected, ARRAY_SIZE(expected));
+
+ TALLOC_FREE(subreq);
+
+ if (!NT_STATUS_IS_OK(status)) {
+ tevent_req_nterror(req, status);
+ return;
+ }
+
+ if (num_bytes != state->data.length) {
+ tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
+ return;
+ }
+
+ if (memcmp(bytes, state->data.data, num_bytes) != 0) {
+ tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
+ return;
+ }
+
+ /* Check pid low/high == DEADBEEF */
+ pidlow = SVAL(phdr, HDR_PID);
+ if (pidlow != 0xBEEF){
+ printf("Incorrect pidlow 0x%x, should be 0xBEEF\n",
+ (unsigned int)pidlow);
+ tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
+ return;
+ }
+ pidhigh = SVAL(phdr, HDR_PIDHIGH);
+ if (pidhigh != 0xDEAD){
+ printf("Incorrect pidhigh 0x%x, should be 0xDEAD\n",
+ (unsigned int)pidhigh);
+ tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
+ return;
+ }
+
+ tevent_req_done(req);
+}
+
+static NTSTATUS pid_echo_recv(struct tevent_req *req)
+{
+ return tevent_req_simple_recv_ntstatus(req);
+}
+
+static bool run_pidhigh(int dummy)
+{
+ bool success = false;
+ struct cli_state *cli = NULL;
+ NTSTATUS status;
+ struct tevent_context *ev = NULL;
+ struct tevent_req *req = NULL;
+ TALLOC_CTX *frame = talloc_stackframe();
+
+ printf("starting pid high test\n");
+ if (!torture_open_connection(&cli, 0)) {
+ return false;
+ }
+ smbXcli_conn_set_sockopt(cli->conn, sockops);
+
+ ev = samba_tevent_context_init(frame);
+ if (ev == NULL) {
+ goto fail;
+ }
+
+ req = pid_echo_send(frame, ev, cli);
+ if (req == NULL) {
+ goto fail;
+ }
+
+ if (!tevent_req_poll_ntstatus(req, ev, &status)) {
+ goto fail;
+ }
+
+ status = pid_echo_recv(req);
+ if (NT_STATUS_IS_OK(status)) {
+ printf("pid high test ok\n");
+ success = true;
+ }
+
+ fail:
+
+ TALLOC_FREE(frame);
+ torture_close_connection(cli);
+ return success;
+}
+
static bool run_local_substitute(int dummy)
{
bool ok = true;
@@ -10197,6 +10355,7 @@ static struct {
{ "CLEANUP3", run_cleanup3 },
{ "CLEANUP4", run_cleanup4 },
{ "OPLOCK-CANCEL", run_oplock_cancel },
+ { "PIDHIGH", run_pidhigh },
{ "LOCAL-SUBSTITUTE", run_local_substitute, 0},
{ "LOCAL-GENCACHE", run_local_gencache, 0},
{ "LOCAL-TALLOC-DICT", run_local_talloc_dict, 0},
--
2.7.4
More information about the samba-technical
mailing list