Subject: Re: Patch to ticket 228

Re: Patch to ticket 228

From: Jernej Kovacic <jkovacic_at_gmail.com>
Date: Tue, 20 Sep 2011 23:50:54 +0200

Hi

The suggested functionality is desired in my application, I believe it
could be useful to the others, so I decided to share it.

BTW, it looks it helped me to discover another bug which I am
investigating right now.

Now let me answer the questions:

On Mon, Sep 19, 2011 at 11:40 PM, Alexander Lamaison <swish_at_lammy.co.uk> wrote:
> The function is called libssh2_session_supported_algs but it doesn't
> actually take a session argument.  I'm not familiar with what this
> function is designed to do.  How is it's task independent of the
> session?

The function returns a list of supported algorithms for each method
(e.g. symetric encryption, HMAC etc.) which depends on the chosen
cryptographic library, configure options, probably on libssh2 version
etc. The function is typically called before a session is established
and, if you take a look into its implementation in kex.c, you'll
notice that it doesn't need anything from LIBSSH2_SESSION. Well, it
could be an option to pass a session argument too and not use it at
all, but IMHO there's no point in it at all.

Or do you mean that the function should be renamed?

> ...
>> +int libssh2_session_supported_algs(int method_type, char** algs, unsigned int nalgs);
>> +.SH DESCRIPTION
>> +Get a list of supported algorithms for each method_type. The method_type parameter is equivalent
>> +to method_type in \fIlibssh2_session_method_pref(3)\fP. Algs must be user preallocated array of pointers to strings
>> +(e.g. char* methods[N_METHODS]) and nalgs is a number of preallocated elemnts in this array.
>> +If successful, the function will fill this array with supported algorithms (the same names as defined in RFC 4253)
>> +and terminate it with a NULL pointer.
>
> Who is responsible for freeing the strings added to the array?  How
> does the caller know how big to make the array?

No strings are added to the array, only pointers to existing strings
(they were created during compile time and are used by
libssh2_session_method_prefs as well) which were initially not visible
to the application. As no extra memory is (m)allocated, there is no
need to free anything.
The caller must guess and provide a reasonable size (20 elements
should be more than enough) of this array. If the array is too short,
an error will be returned. Of course I made every effort to prevent
any buffer overflow in this case.

>> +.SH RETURN VALUE
>> +On success, a number of returned algorithms (i.e a positive number will be returned).
>
> What number?  The number added to the array?  Is this not fixed?  If
> not, why does the function not allocate the array itself so that it's
> the right size?

Even I wasn't quite sure what to return on success. One option would
be strictly returning 0 or a negative error code in case of a failure.
Then I noticed I can return a value of a working variable (from
implementation in kex.c) and decided to return it as it represents a
number of returned algorithms in algs. Anyway, this is indeed not
particularly useful, so it's your decision.....

>> --- a/include/libssh2.h
>> +++ b/include/libssh2.h
>> +/*
>> +    Retrieve a list of supported algorithms for given method_type (see ibssh2_session_method_pref)
>> +        algs  - user preallocated array of pointers to string which will be filled (and NULL terminated) in by the function
>> +             nalgs - number of reserved elements in algs, incl. the NULL termination
>
> Erm ... why would nalgs be NULL terminated?  It's an array of pointers
> and its size is passed around explicitly.

Your question is not clear, possibly due a typing error. (algs will be
NULL terminated, not nalgs which is an integer). Well, this means if 7
algorithms are returned, algs[0 to 6] will be pointers to their names,
algs[7] will be NULL, indicating that algs[8] to algs[nalgs-1]
(remaining elements of the preallocated array) are not "interesting"
at all.

>> +LIBSSH2_API int libssh2_session_supported_algs(int method_type, char** algs, unsigned int nalgs);
>
> We should use size_t for all new APIs.  unsigned int only exists in
> the current API to maintain ABI compatability.  They will also be
> changed to use size_t at the next soname bump.

OK, I agree.

I hope some issues are now clarified.

Regards, Jernej

_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2011-09-20