> -----Original Message-----
> From: curl-library [mailto:curl-library-bounces_at_cool.haxx.se] On Behalf Of
> Daniel Stenberg
> Sent: Tuesday, September 30, 2014 3:20 AM
> To: libcurl development
> Subject: Re: [PATCH] smb/cifs
>
> On Mon, 29 Sep 2014, Nagel, Bill wrote:
>
> > This patch implements the CIFS dialect of SMB.
>
> Thanks a lot for your work on this! I'm impressed the patch isn't bigger, but I
> have a feeling it'll grow...
>
> Some additional comments on top of what Steve already mentioned. In
> general I think you need to consider that you're donating this code to a
> project for the long term. We, and future hackers like us, want to be able to
> read and understand code we merge for a long time into the future. We
> need code to be readable and understandable.
>
> 1 - your patch leaks memory in OOM situations (we want perfect cleanup in
> all
> error situations as well), this means that if you do 3 memory allocations
> and the 3rd call fails, the first two must free their memory when the
> failure is returned.
>
> 2 - the magic constant 0x11000 is used on several places without motivation
> or
> explanation. Why can't it be smaller? Why doesn't it have to be bigger?
>
> 3 - 'make checksrc' will point out some formatting nits for you
>
> 4 - In smb_send_setup
>
> A) what are the "Linux" and "curl_" parts used for?
>
> B) lots of uncommented constants in that code. 0xff and 0x73 for example.
> What about using defines with suitable names instead? Other functions
> use things like 0x71 and 0x2e. What do they mean?
>
> C) are those strcpy() calls there really safely bounded so there's no risk
> even if I send in a 10K long user name? Doesn't look like that! The same
> goes for the strcpy() calls in smb_send_tree_connect() and some other
> places. Every single use of strcpy() needs to be checked for length and
> boundaries and preferably a comment next to it should mention how it is
> OK.
>
> 5 - What are your thoughts around test cases for this? Without tests we
> know
> that code bitrots and breaks over time. How can we get a simple test
> server for this?
>
> --
>
> / daniel.haxx.se

Thanks for the comments. The patch just implements the basics, there are still more features I want to add. Those strings identify OS and client, so I need a better way to detect OS.

I've been looking for a simple server to test against, but there isn't much out there. There is a perl-smb server, but it appears to only implement smb2. There is also a python project called impacket that includes an smb server that looks promising. I will turn my attention to the tests next, and any help will be appreciated.

Once testing is implemented, I plan on adding Kerberos support and range based uploads.

-Bill

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-09-30