[ldns-users] ldns 1.7.0 rc3

[Willem Toorop]

Thank you Michael,

That the ldns_tsig_credentials type is not actually used in any function
of the API (besides the access and clone functions) is indeed a bit
peculiar.  I do not know what the intention was originally, but they
have been exposed in earlier releases and they don't do any harm either,
so I'd rather not remove them.

I've applied your patches to update the python bindings; and did the
perl bindings myself.
Cheers,

-- Willem


Op 18-12-16 om 18:51 schreef Michael Weiser:
> Hi Willem,
> 
> On Thu, Dec 15, 2016 at 05:54:29PM +0100, Willem Toorop wrote:
> 
>> GOST support in newer OpenSSLs and an issue with how the language
>> bindings deal with variables that have become const.
> 
> Since I was the one who created it, I today had a look at the
> python-const situation. I get two sets of warnings:
> 
> 1. ldns_tsig_credentials_struct:
> 
> ldns/tsig.h:28: Warning 451: Setting a const char * variable may leak memory.
> ldns/tsig.h:29: Warning 451: Setting a const char * variable may leak memory.
> ldns/tsig.h:30: Warning 451: Setting a const char * variable may leak memory.
> 
> These are from wrapping ldns_tsig_credentials_struct into a python
> object. From what I understand it boils down to the fact that swig
> doesn't dare free() the old value when setting a new one but also
> doesn't dare not strdup()ing the new value.
> 
> This can be silenced  by something like this:
> 
> --- a/contrib/python/ldns.i
> +++ b/contrib/python/ldns.i
> @@ -126,6 +126,9 @@ uint32_t ldns_read_timeval_usec(struct timeval* t) {
>  %immutable ldns_struct_rr_descriptor::_name;
>  %immutable ldns_error_str;
>  %immutable ldns_signing_algorithms;
> +%immutable ldns_tsig_credentials_struct::algorithm;
> +%immutable ldns_tsig_credentials_struct::keyname;
> +%immutable ldns_tsig_credentials_struct::keydata;
>  
>  //*_new_frm_fp_l
>  %apply int *OUTPUT { (int *line_nr) };
> 
> This turns ldns_tsig_credentials_struct read-only (no setter methods are
> created) and in consequence most likely totally useless as a python
> object. It could be used to safely interface to a bit of C code though
> that initialised its own ldns_tsig_credentials_struct any odd way and
> use the contents from python, if (I don't know how) this initialised
> struct can be turned into a python object.
> 
> Alternatively a swig typemap could be written that either passes the
> literal pointers around not caring about duplicating them or basically
> ignores the const and restores behaviour of free()ing the values when
> new ones are set. If the python binding is meant only ever to be used
> *from* python, both solutions should be fine. But if someone wanted to
> use it to interact with a bit of C code that might have done anything
> to ldns_tsig_credentials_struct previously, it creates exactly the same
> dilemma, swig is telling us about with this warning.
> 
> The clean way[tm] would then be to make the object opaque to C users as
> well and have _init, _free and _set functions comparable to
> ldns_resolver and other ldns objects.
> 
> Which begs the question I have been asking myself on the level of C
> programming as well: What *is* ldns_tsig_credentials_struct actually
> good for other than keeping those three parameters neatly together in
> the user application? Are there actually any known users other than
> examples/ldns-update.c? I'd expect most user applications to elect to
> handle those parameters their own way anyway, leaving only potential for
> confusion and boilerplate code...
> 
> So I have to ask: Might it be the simplest fix to simply drop the whole
> concept? Suggested patch attached.
> 
> Or was the usage of ldns_tsig_credentials_struct supposed to be extended
> to pass all three parameters to ldns in one go?
> 
> 2. resolver:
> ./contrib/python/ldns_wrapper.c: In function '_ldns_resolver_tsig_algorithm':
> ./contrib/python/ldns_wrapper.c:3609:9: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>      str = ldns_resolver_tsig_algorithm(res);
>          ^
> ./contrib/python/ldns_wrapper.c: In function '_ldns_resolver_tsig_keydata':
> ./contrib/python/ldns_wrapper.c:3669:9: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>      str = ldns_resolver_tsig_keydata(res);
>          ^
> ./contrib/python/ldns_wrapper.c: In function '_ldns_resolver_tsig_keyname':
> ./contrib/python/ldns_wrapper.c:3680:9: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>      str = ldns_resolver_tsig_keyname(res);
> 
> Those can easily be fixed like this, it seems:
> 
> diff --git a/contrib/python/ldns_resolver.i b/contrib/python/ldns_resolver.i
> index b926e65a..7081ec36 100644
> --- a/contrib/python/ldns_resolver.i
> +++ b/contrib/python/ldns_resolver.i
> @@ -113,9 +113,9 @@
>  %rename(__ldns_resolver_tsig_algorithm) ldns_resolver_tsig_algorithm;
>  %inline
>  %{
> -  char * _ldns_resolver_tsig_algorithm(const ldns_resolver *res)
> +  const char * _ldns_resolver_tsig_algorithm(const ldns_resolver *res)
>    {
> -    char *str;
> +    const char *str;
>      str = ldns_resolver_tsig_algorithm(res);
>      if (str != NULL) {
>        str = strdup(str);
> 
> (full patch attached).
> 
> I hope this is helpful, if only perhaps as a second opinion. :)
>