Created attachment 5771 [details]
proposed patch so create works.

Hmmmm. Can you let me know how you saw this ? The problem with this patch is that it breaks the boilerplate setup for iovecs i and i+1, which may have dependencies in other places. What do the mangled compound replies look like ? Can you attach a wireshark trace or log ?
Jeremy.

The code for i is untouched...

i+1 and i+2 are changed.  What boilerplate am I impacting?

-Ira

In smbd_smb2_request_setup_out() (which initializes all outvecs) we have:

                outhdr = talloc_zero_array(vector, uint8_t,
                                      OUTVEC_ALLOC_SIZE);
                if (outhdr == NULL) {
                        return NT_STATUS_NO_MEMORY;
                }

                outbody = outhdr + SMB2_HDR_BODY;

                current[0].iov_base     = (void *)outhdr;
                current[0].iov_len      = SMB2_HDR_BODY;

                current[1].iov_base     = (void *)outbody;
                current[1].iov_len      = 8;

Plus inside smbd_smb2_request_error_ex() we have the comment:

                /* Allocated size of req->out.vector[i].iov_base
                 * *MUST BE* OUTVEC_ALLOC_SIZE. So we have room for
                 * 1 byte without having to do an alloc.
                 */

Also, the code you replaced is also duplicated inside smbd_smb2_request_pending_queue(), so if it's wrong I really want to understand *why* it's wrong :-). I'm not saying your patch is incorrect, I just don't understand why the original code screws up and that scares me :-).

Jeremy.

The boilerplate should be safe here... because the copy will keep the length of the field being copied from.  And that comment should be at LEAST...  i+1 is certainly changed in the code I thought?

I'll see what I can do to get a reproduction.

Hang on a minute... I see the problem (I think). How about this as a minimal patch ?

diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index df25570..009cc77 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -551,9 +551,7 @@ static struct smbd_smb2_request *dup_smb2_req(const struct smbd_smb2_request *re
                        outvec[i+2].iov_base = ((uint8_t *)outvec[i].iov_base) +
                                (OUTVEC_ALLOC_SIZE - 1);
                        outvec[i+2].iov_len = 1;
-               } else if (!dup_smb2_vec(outvec,
-                               req->out.vector,
-                               i)) {
+               } else if (!dup_smb2_vec(outvec, req->out.vector, i+2)) {
                        break;
                }
        }

Created attachment 5772 [details]
Minimal patch ?

Arg. Mangled by the text field. This is what I meant..

Ok, I'm pretty sure this will fix it - that's the core of the problem (I was duplicating vector i, instead of vector i+2). The other part of your patch (duplicating vector i+1) is correct but I think unnecessary. I'm going to check in the minimal patch under your name, and if it doesn't fix the problem I'll investigate further and possibly add in the second part (once I understand the problem fully :-). But I think this should fix it.

Thanks once again :-) :-). I am very impressed with how much you test our code. Let me know if I can do anything for you guys as thanks.

Jeremy.

Still broken... Do not push.

-Ira

Dam, too late (sorry). Can we chat on the phone ? I really need to understand this one..

Jeremy.

Ahhhh. *Now* I get it (looking really carefully at the create code...).
This needs more work. Sorry for the early push. I'll credit you with all changes.

Jeremy.

In reading the code... I think my changes are right in spirit.  You shouldn't
be assuming what is in outvec[i] has anything in common with outvec[i+1] or
outvec[i+2].

You may want to assume i+1 has a non-zero length, and that's fine if
initialized that way... but in a general case... i+1 and i+2 can be whatever
the call deems it wants I thought.  Even i is really that way, but by
convention we put the header in there only... But I haven't seen anything
enforce that rule.

The code as is when I read it looks more and more wrong... a simpler check for
non-zero length and if so, just copy the vector seems right.

-Ira

Created attachment 5773 [details]
Proposed patch.

Thanks for being patient with me. This is the patch that should work for all cases. It contains your original fix (with a few extra comments), plus an adjustment to the vector calculations in smbd_smb2_request_pending_queue().

Please let me know if this fixes the problems.

Jeremy.

I'll get it tested tomorrow.

As far as the patch goes:

Now the code uses two techniques, I'd just use one or the other, probably the one in smbd_smb2_request_pending_queue which is more efficient.  (though more confusing.)

Is there a way to make one function that has all this logic in it, instead of duplicating it?

-Ira

Hmmm. Good point, I'll look at that.

Jeremy.

Created attachment 5774 [details]
Refactored patch

Ok, that was a very good idea of yours :-). Refactored patch abstracting the change out to a function dup_smb2_vec3(), that replaces dup_smb2_vec(). Should fix the bug and is *much* cleaner :-). Thanks !

Please test.

Jeremy.

Patch looks good, and works.  (assuming that's what is on master now :) )

-Ira

Yep - that's what is in master now. Closing this one out - thanks a *lot* for the help !
Jeremy.