Re: [Freeipa-devel] [PATCH 0234] Prevent NULL dereference before sync_concurr_limit_signal() calls
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/09/2014 02:07 PM, Petr Spacek wrote: Hello, Prevent NULL dereference before sync_concurr_limit_signal() calls. Missing check was causing NULL dereference in case where manager_get_ldap_instance() failed. This typically happens when BIND is processing LDAP updates during shutdown. I noticed this crash during sanity testing 4.2 release... Please review it ASAP so I can release 4.3. How to reproduce the problem: Run BIND manually from console: $ named -4 -g -u named -m record -n 10 and press Ctrl+C almost immediately. Sometimes it shutdowns cleanly and sometimes you can see a crash: Thank you for your time! ACK. I'm not able to reproduce the issue, but the patch looks reasonable and should not break anything. Regards, Tomas Hozza -BEGIN PGP SIGNATURE- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJTRUklAAoJEMWIetUdnzwtSSoH/0ZIz+MZfZ1O9JH5kGvJQKQo 3s1Fqeh601ReRKGnFu+nBt5hXzGzDOjsXM0x8bJH5r1cABn9XzYjupVQg0FjziM1 XIoJaDRB8L09sjLefjPoY87x0K6OsEQ/EmipDZPQB62MPpWJdGkJi6u2ug2gguRN fEdMeFdvm6T8faU/POd1D/1mEky2DqzNXViLf+6Pdi03b6lvUzoOvqsLwh4+Npvw NVUw7+SzwylCJQBcmZShCL6XRnaN4Qe9LCUG997HNwF/n3K1NjUgbwVqqmdLPGXh XAtcQxic5TQkKl2A1+52YFLMLSLY5gfAg/qBYmT1BQM+IuUIUYOe7fXL0fxr+A0= =TNAe -END PGP SIGNATURE- ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0234] Prevent NULL dereference before sync_concurr_limit_signal() calls
On 9.4.2014 15:20, Tomas Hozza wrote:
On 04/09/2014 02:07 PM, Petr Spacek wrote:
Hello,
Prevent NULL dereference before sync_concurr_limit_signal() calls.
Missing check was causing NULL dereference in case where
manager_get_ldap_instance() failed. This typically happens when BIND
is processing LDAP updates during shutdown.
I noticed this crash during sanity testing 4.2 release...
Please review it ASAP so I can release 4.3.
How to reproduce the problem:
Run BIND manually from console:
$ named -4 -g -u named -m record -n 10
and press Ctrl+C almost immediately.
Sometimes it shutdowns cleanly and sometimes you can see a crash:
Thank you for your time!
ACK.
I'm not able to reproduce the issue, but the patch looks reasonable and
should not break anything.
Thanks. I have modified the patch once again before push to silence Clang
warnings about potential NULL-inst deference on following lines:
if (dns_name_dynamic(name))
dns_name_free(name, inst-mctx);
In reality the NULL dereference cannot happen because it is guarded by
condition in dns_name_dynamic().
This is not a problem now but the behavior depends on internal implementation
in BIND. It is definitely better to add explicit check to stay safe ...
--
Petr^2 Spacek
From cb9ed81e6b1fdd26a06739fe819b730eb4a70839 Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Wed, 9 Apr 2014 14:01:00 +0200
Subject: [PATCH] Prevent NULL dereference before sync_concurr_limit_signal()
calls.
Missing check was causing NULL dereference in case where
manager_get_ldap_instance() failed. This typically happens when BIND
is processing LDAP updates during shutdown.
Signed-off-by: Petr Spacek pspa...@redhat.com
---
NEWS | 5 +
src/ldap_helper.c | 35 ---
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/NEWS b/NEWS
index e787e7f2d73e3e99d3d5c0d03b9ea92dff75b510..78843f2d036a7ab25c00924af91e63228aea0a8f 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,8 @@
+4.3
+
+[1] LDAP update processing was fixed to prevent BIND from crashing during
+shutdown.
+
4.2
[1] Record parsing was fixed to prevent child-zone data corruption in cases
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 678e9f8a52181a5c63c96d29da9b3e5ec3b1273d..cf8d45ecc6ef24653be731020703c24c9b4c7214 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -3913,16 +3913,18 @@ update_zone(isc_task_t *task, isc_event_t *event)
}
cleanup:
- sync_concurr_limit_signal(inst-sctx);
+ if (inst != NULL) {
+ sync_concurr_limit_signal(inst-sctx);
+ if (dns_name_dynamic(currname))
+ dns_name_free(currname, inst-mctx);
+ if (dns_name_dynamic(prevname))
+ dns_name_free(prevname, inst-mctx);
+ }
if (result != ISC_R_SUCCESS)
log_error_r(update_zone (syncrepl) failed for '%s'.
Zones can be outdated, run `rndc reload`,
pevent-dn);
- if (dns_name_dynamic(currname))
- dns_name_free(currname, inst-mctx);
- if (dns_name_dynamic(prevname))
- dns_name_free(prevname, inst-mctx);
isc_mem_free(mctx, pevent-dbname);
if (pevent-prevdn != NULL)
isc_mem_free(mctx, pevent-prevdn);
@@ -3948,7 +3950,8 @@ update_config(isc_task_t *task, isc_event_t *event)
CHECK(ldap_parse_configentry(entry, inst));
cleanup:
- sync_concurr_limit_signal(inst-sctx);
+ if (inst != NULL)
+ sync_concurr_limit_signal(inst-sctx);
if (result != ISC_R_SUCCESS)
log_error_r(update_config (syncrepl) failed for '%s'.
Configuration can be outdated, run `rndc reload`,
@@ -4200,17 +4203,19 @@ cleanup:
pevent-dn, pevent-chgtype);
}
- sync_concurr_limit_signal(inst-sctx);
+ if (inst != NULL) {
+ sync_concurr_limit_signal(inst-sctx);
+ if (dns_name_dynamic(name))
+ dns_name_free(name, inst-mctx);
+ if (dns_name_dynamic(prevname))
+ dns_name_free(prevname, inst-mctx);
+ if (dns_name_dynamic(origin))
+ dns_name_free(origin, inst-mctx);
+ if (dns_name_dynamic(prevorigin))
+ dns_name_free(prevorigin, inst-mctx);
+ }
if (zone_ptr != NULL)
dns_zone_detach(zone_ptr);
- if (dns_name_dynamic(name))
- dns_name_free(name, inst-mctx);
- if (dns_name_dynamic(prevname))
- dns_name_free(prevname, inst-mctx);
- if (dns_name_dynamic(origin))
- dns_name_free(origin, inst-mctx);
- if (dns_name_dynamic(prevorigin))
- dns_name_free(prevorigin, inst-mctx);
ldapdb_rdatalist_destroy(mctx, rdatalist);
isc_mem_free(mctx, pevent-dbname);
if (pevent-prevdn != NULL)
--
1.9.0
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0234] Prevent NULL dereference before sync_concurr_limit_signal() calls
On (09/04/14 16:38), Petr Spacek wrote: On 9.4.2014 15:20, Tomas Hozza wrote: On 04/09/2014 02:07 PM, Petr Spacek wrote: Hello, Prevent NULL dereference before sync_concurr_limit_signal() calls. Missing check was causing NULL dereference in case where manager_get_ldap_instance() failed. This typically happens when BIND is processing LDAP updates during shutdown. I noticed this crash during sanity testing 4.2 release... Please review it ASAP so I can release 4.3. How to reproduce the problem: Run BIND manually from console: $ named -4 -g -u named -m record -n 10 and press Ctrl+C almost immediately. Sometimes it shutdowns cleanly and sometimes you can see a crash: Thank you for your time! ACK. I'm not able to reproduce the issue, but the patch looks reasonable and should not break anything. Thanks. I have modified the patch once again before push to silence Clang warnings about potential NULL-inst deference on following lines: if (dns_name_dynamic(name)) dns_name_free(name, inst-mctx); In reality the NULL dereference cannot happen because it is guarded by condition in dns_name_dynamic(). This is not a problem now but the behavior depends on internal implementation in BIND. It is definitely better to add explicit check to stay safe ... -- Petr^2 Spacek From cb9ed81e6b1fdd26a06739fe819b730eb4a70839 Mon Sep 17 00:00:00 2001 From: Petr Spacek pspa...@redhat.com Date: Wed, 9 Apr 2014 14:01:00 +0200 Subject: [PATCH] Prevent NULL dereference before sync_concurr_limit_signal() calls. Missing check was causing NULL dereference in case where manager_get_ldap_instance() failed. This typically happens when BIND is processing LDAP updates during shutdown. Signed-off-by: Petr Spacek pspa...@redhat.com --- scan-build: No bugs found. ACK LS ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0234] Prevent NULL dereference before sync_concurr_limit_signal() calls
On 9.4.2014 17:39, Lukas Slebodnik wrote: On (09/04/14 16:38), Petr Spacek wrote: On 9.4.2014 15:20, Tomas Hozza wrote: On 04/09/2014 02:07 PM, Petr Spacek wrote: Hello, Prevent NULL dereference before sync_concurr_limit_signal() calls. Missing check was causing NULL dereference in case where manager_get_ldap_instance() failed. This typically happens when BIND is processing LDAP updates during shutdown. I noticed this crash during sanity testing 4.2 release... Please review it ASAP so I can release 4.3. How to reproduce the problem: Run BIND manually from console: $ named -4 -g -u named -m record -n 10 and press Ctrl+C almost immediately. Sometimes it shutdowns cleanly and sometimes you can see a crash: Thank you for your time! ACK. I'm not able to reproduce the issue, but the patch looks reasonable and should not break anything. Thanks. I have modified the patch once again before push to silence Clang warnings about potential NULL-inst deference on following lines: if (dns_name_dynamic(name)) dns_name_free(name, inst-mctx); In reality the NULL dereference cannot happen because it is guarded by condition in dns_name_dynamic(). This is not a problem now but the behavior depends on internal implementation in BIND. It is definitely better to add explicit check to stay safe ... -- Petr^2 Spacek From cb9ed81e6b1fdd26a06739fe819b730eb4a70839 Mon Sep 17 00:00:00 2001 From: Petr Spacek pspa...@redhat.com Date: Wed, 9 Apr 2014 14:01:00 +0200 Subject: [PATCH] Prevent NULL dereference before sync_concurr_limit_signal() calls. Missing check was causing NULL dereference in case where manager_get_ldap_instance() failed. This typically happens when BIND is processing LDAP updates during shutdown. Signed-off-by: Petr Spacek pspa...@redhat.com --- scan-build: No bugs found. ACK Thanks! Pushed to master: cb9ed81e6b1fdd26a06739fe819b730eb4a70839 -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
