[PATCHSET] smb2 durable handle torture tests and server fixes

[Jeremy Allison]

On Thu, Sep 26, 2013 at 08:49:44AM +0200, Michael Adam wrote:
> Hi,
> 
> attached find a patchset for master that extends and fixes
> a couple of smb2 tests (durable handles). The server is
> fixed as needed to pass these.
> 
> This is the result of running ms protocol tests and studying
> the docs. Also the lease-versions of durable tests now
> pass against windows... (*cough*).
> 
> A couple of the first patches have already been reviewed by
> Metze.
> 
> Patches are also found in my master-smb2 branch:

OK, I've gone through these carefully. There are
some instances of using :

talloc_free(tree);
tree = NULL;

which would probably be better served by
TALLOC_FREE(tree);

But the place I'd like to see a little
cleanup is in the patches at the end that
do :

+       dhnq = smb2_create_blob_find(&in_context_blobs,
+                                    SMB2_CREATE_TAG_DHNQ);
        dhnc = smb2_create_blob_find(&in_context_blobs,
                                     SMB2_CREATE_TAG_DHNC);
+       dh2q = smb2_create_blob_find(&in_context_blobs,
+                                    SMB2_CREATE_TAG_DH2Q);
+       dh2c = smb2_create_blob_find(&in_context_blobs,
+                                    SMB2_CREATE_TAG_DH2C);
+
+       if ((dhnc && dh2c) || (dhnc && dh2q) || (dh2c && dhnq) ||
+           (dh2q && dh2c))
+       {
+               /* not both are allowed at the same time */
+               tevent_req_nterror(req, NT_STATUS_INVALID_PARAMETER);
+               return tevent_req_post(req, ev);
+       }

and then do:

+               uint32_t num_extra_blobs;
                if (dhnc->data.length != 16) {
                        tevent_req_nterror(req, NT_STATUS_INVALID_PARAMETER);
                        return tevent_req_post(req, ev);
                }
-               if (in_context_blobs.num_blobs != 1) {
+
+               num_extra_blobs = in_context_blobs.num_blobs - 1;
+               if (dhnq) {
+                       /*
+                        * Note: We ignore additional dhnq blob according
+                        * to MS-SMB2: 3.3.5.9.7 Handling the
+                        * SMB2_CREATE_DURABLE_HANDLE_RECONNECT Create Context
+                        */
+                       num_extra_blobs--;
+               }
+
+               if (num_extra_blobs > 0) {

Now I've looked at your logic here on decrementing
num_extra_blobs, which is unsigned, and I think
it's right.

Problem is it looks a little dodgy and hard to
follow. I'd like to see a comment around the

num_extra_blobs = in_context_blobs.num_blobs - 1;

saying something like

"If the dhnc pointer is set we can guarentee that
in_context_blobs.num_blobs is at least 1 */

and then something similar around the decrement
of num_extra_blobs protected by the "if (dhnq)"
check.

We really want to make it clear that uint32_t num_extra_blobs
can never wrap lower (higher) than zero.

Other than that - looks good !

Cheers,

	Jeremy.