On Fri, 18 Dec 2015, Joseph Tetzlaff-Deas wrote:

> Hopefully you can consider this patch for inclusion into the Curl libraries.
> If you have any questions about this functionality, let me know and I can go
> into more detail on the changes I've made.

Thanks a lot for your contribution. It need some little more polish though.

In addition to Gigle's remarks I found a few things I think need attention:

> static void smb_sign(struct connectdata *conn, struct smb_conn *smbc,

No return code? It uses functions that can fail so it should! And the users of
this function then needs to check the return code etc.

> full_msg = conn->data->state.uploadbuffer + 4;
> security_features = conn->data->state.uploadbuffer + 18;

I'm not a champion of the SMB code. Can we really be certain that the upload
buffer is always having that data at that index at this point?

> memcpy(MAC_key, smbc->MAC_key, sizeof(MAC_key));

What is the point of this copy, since you copy it to another destination again
just 6 lines below? I don't see what the Mac_key local array is needed for?

> /* calculate MAC signature */
> /* concat the MAC key and entire SMB message */
> MAC_data = malloc(sizeof(MAC_key) + msg_len +
> sizeof(struct smb_header) - 4);

But why the -4 ? Please add a comment to explain.

Also, malloc() can fail and we always check for that in libcurl. You need to
handle that and bail out nicely with no memory leaks. Thanks!

> - msg.session_key = smb_swap32(smbc->session_key);
> + msg.session_key = smb_swap32(SMB_SESSION_KEY);

Really? This makes it seem like this was totally broken before. Was it? Or how
come you can replace a dynamic key use with a fixed one like that?

2. Some compiler complaints that I get when trying a build with your patch
applied: (I recommend "./configure --enable-debug --enable-werror" to discover
them yourself)

smb.c: In function 'smb_sign':
smb.c:301:21: error: pointer targets in assignment differ in signedness
[-Werror=pointer-sign]
    security_features = conn->data->state.uploadbuffer + 18;
                      ^
smb.c: In function 'smb_send_write':
smb.c:684:23: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
    nread = upload_size > (BUFSIZE - msgsize) ? (BUFSIZE - msgsize) :
                        ^
smb.c:684:67: error: signed and unsigned type in conditional expression
[-Werror=sign-compare]
    nread = upload_size > (BUFSIZE - msgsize) ? (BUFSIZE - msgsize) :
                                                                    ^
smb.c:684:11: error: conversion to 'int' from 'size_t {aka long unsigned int}'
may alter its value [-Werror=conversion]
    nread = upload_size > (BUFSIZE - msgsize) ? (BUFSIZE - msgsize) :
            ^

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html