[Freeipa-devel] [PATCH 0149] Clean up PTR record synchronization code and make it more robust

[Petr Spacek]

On 13.5.2013 15:23, Lukas Slebodnik wrote:
> On (06/05/13 14:03), Petr Spacek wrote:
>> On 18.4.2013 11:04, Petr Spacek wrote:
>>> Hello,
>>>
>>> Clean up PTR record synchronization code and make it more robust.
>>>
>>> PTR record synchronization was split to smaller functions.
>>> Input validation, error handling and logging was improved
>>> significantly.
>>
>> Tbabej's GCC cries about uninitialized variable 'ptr_a_equal', but we
>> weren't able to find any real error.
>>
>> This version of the patch contains a workaround for the GCC oddities.
>>
>> --
>> Petr^2 Spacek
>
>>From 5e6abb29df58ce00ecf7045254dfc7fb09fc4650 Mon Sep 17 00:00:00 2001
>> From: Petr Spacek <pspacek at redhat.com>
>> Date: Tue, 16 Apr 2013 16:10:09 +0200
>> Subject: [PATCH] Clean up PTR record synchronization code and make it more
>> robust.
>>
>> PTR record synchronization was split to smaller functions.
>> Input validation, error handling and logging was improved
>> significantly.
>>
>> Signed-off-by: Petr Spacek <pspacek at redhat.com>
>> ---
>> src/ldap_helper.c | 507 ++++++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 342 insertions(+), 165 deletions(-)
>>
>> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
>> index 8448412b7a1a9150bd24d9ca46575c0402be0c9f..6c5cf2e79d762251954e3bb099dbef98a0b2d805 100644
>> --- a/src/ldap_helper.c
>> +++ b/src/ldap_helper.c
>> @@ -2830,35 +2830,360 @@ cleanup:
>> #undef SET_LDAP_MOD
>> }
>>
>> +
>> +#define SYNCPTR_PREF    "PTR record synchronization "
>> +#define SYNCPTR_FMTPRE  SYNCPTR_PREF "(%s) for A/AAAA '%s' "
>> +#define SYNCPTR_FMTPOST ldap_modop_str(mod_op), a_name_str
>> +
>> +static const char *
>> +ldap_modop_str(unsigned int mod_op) {
>> +	static const char add[] = "addition";
>> +	static const char del[] = "deletion";
>                         ^^^^^
> Declaration(definition) of string should be consistent everywhere in the source code.
> Please change "char _name[]" to "char * _name"
> Sorry for nitpicking, I know that semantically is it the same,
> but in the other places in source code, strings are
> defined like "(const)? char *".
> If you don't believe me, you can run next command :-)
>
>      grep -Rn -E 'char.*=.*"[^"]*"' <path_to_bind-dyndb-ldap>
>
> [snip]
>
>> +static isc_result_t
>> +ldap_find_ptr(ldap_instance_t *ldap_inst, const char *ip_str,
>> +	      dns_name_t *ptr_name, ld_string_t *ptr_dn,
>> +	      dns_name_t *zone_name) {
>> +	isc_result_t result;
>> +	isc_mem_t *mctx = ldap_inst->mctx;
>> +
>> +	in_addr_t ip;
>> +
>> +	/* Get string with IP address from change request
>> +	 * and convert it to in_addr structure. */
>> +	if ((ip = inet_addr(ip_str)) t) == 0) {
>                                      ^^^^
> If the input is invalid, INADDR_NONE (usually -1) is returned.
> For details: man inet_addr
>
>> +		log_bug(SYNCPTR_PREF " could not convert IP address "
>> +			"from string '%s'", ip_str);
>> +		CLEANUP_WITH(ISC_R_UNEXPECTED);
>> +	}

Thank you for catching this! Nobody noticed it for one and half year :-)

In any case, this code can't handle IPv6 addresses. We will triage it 
tomorrow: https://fedorahosted.org/bind-dyndb-ldap/ticket/118

Fixed patch is attached. The new version includes also typo fix: " could" => 
"could".

-- 
Petr Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0149-3-Clean-up-PTR-record-synchronization-code-and-make-it.patch
Type: text/x-patch
Size: 19022 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130513/bb066dbe/attachment.bin>