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
