Multiple basic problems with version 4.2.3 OMAPI especially with subclass -- with fixes
Jeff Waller
jeffw at cnxntech.com
Wed Jan 11 21:53:31 UTC 2012
Hello all,
Over the past few weeks I'm been working with version 4.2.3 of the DHCP server as part of our company's
effort to administer DHCP via online method. In particular we have had the need to create and remove
subclasses. What I have found is there are several bugs (some serious) that prevent this. See below
for a discussion of each of those bugs and a number of patches at the end. BTW I considered sending
this to dhcp-hackers, which I suppose I still can do in addition, but the traffic there is practically
low.
Bug #1 Reading a subclass declaration from the leases file generates the wrong type of object.
This bug is caused when the DHCP server is re-started after a subclass is created via OMAPI.
Whenever a subclass is created this way, a subclass definition is written out to the leases file.
Later, in confpars.c, when definition of a subclass or class is read upon DHCPd restart, a class is
generated regardless of whether the definition is a class or a subclass. The fix is simply to check to
see what is the object type that is supposed to be created and then create that type of object.
Bug #2: Wrong type of object is checked for in attribute write of subclass information in the
server when it's sent to an OMAPI client.
This bug arises when subclass information is sent to a OMAPI client. The function responsible for
this subclass_stuff_values(), checks if the object it is about to write out is of the correct type.
Unfortunately it checks to see if that object is of type class not subclass as it should and because
of this, the socket is immediately closed. I suspect this is because the code for subclass_stuff_values()
was cut-and-pasted from class_stuff_values(). The fix is to simply change the check to subclass
rather than class.
Bug #3: Subclasses can not be removed via OMAPI.
This bug is due to a long standing lack of support for subclass removal. There is a comment indicating that
this is not possible due to the way subclasses are implemented, but I think in version 4.2.3, this is no
longer true. The fix is to uncomment the code already in place to make the function dhcp_remove_subclass
actually remove a subclass rather than return "NOT_IMPLEMENTED" We have tested this pretty thoroughly
and found the resulting server stable, but it requires 1 more check (see below).
Bug #4: OMAPI client message parser considers transmission of zero-attributed objects invalid.
The effect is that subclasses, classes, and pools can not be read via OMAPI. I believe this to be a bug
in the parser, but am not familiar enough with OMAPI to say it an OMAPI object message
for a object with 0 attributes can actually be distinguished from an invalid OMAPI message. So I have implemented
a workaround and changed dhcp_pool_stuff_values, dhcp_class_stuff_values, and dhcp_subclass_stuff_values
to send 1 key value pair rather that 0. though extraneous, this at least makes the parser happy.
Bug #5: Subclass removal does not remove the class hash entry
The effect is that the DHCP server behaves as if subclasses are never removed. When a subclass or a class
is removed via OMAPI, the fuction delete_class in mdb.c is called. This function did not behave
differently if the type of object was a class or subclass but needed to. This fix is to additionally remove the
class hash if the object is a subclass.
Bug #6: Subclass group created via OMAPI set to the root group.
the effect of this bug is that any DHCP response that does in fact match a subclass or class (via match hardware)
will not carry with it options or other attributes defined in the class but rather the root group. The fix is to
copy the classes group to the subclass.
===============================================================
I've also added a new feature and it's hard to dissociate it from the bug patches.
New feature:
Lease hardware address can be set via OMAPI. Basically, a new lease is cloned from the current lease, the
hardware address is set to whatever, and supersede_lease is called. This pattern is probably necessary
to avoid the shortcut behavior of supersede_lease when it simply moves a lease (the HW address need to be
re-hashed).
================================================================
*** omapi.c Wed Jan 11 14:48:43 2012
--- omapi.c.orig Tue Dec 20 19:43:05 2011
***************
*** 231,248 ****
ols = "unknown state";
if (lease -> binding_state != bar) {
!
! struct lease *new_lease = 0;
!
! if(!lease_copy(&new_lease,lease,MDL))
! {
! log_info("unable to copy lease");
! return ISC_R_IOERROR;
! }
!
! new_lease -> next_binding_state = bar;
! if (supersede_lease (lease,new_lease, 1, 1, 1)) {
!
log_info ("lease %s state changed from %s to %s",
piaddr(lease->ip_addr), ols, nls);
return ISC_R_SUCCESS;
--- 231,238 ----
ols = "unknown state";
if (lease -> binding_state != bar) {
! lease -> next_binding_state = bar;
! if (supersede_lease (lease, 0, 1, 1, 1)) {
log_info ("lease %s state changed from %s to %s",
piaddr(lease->ip_addr), ols, nls);
return ISC_R_SUCCESS;
***************
*** 303,335 ****
} else if (!omapi_ds_strcmp (name, "billing-class")) {
return DHCP_R_UNCHANGED; /* XXX carefully allow change. */
} else if (!omapi_ds_strcmp (name, "hardware-address")) {
! if (value && (value -> type == omapi_datatype_data || value -> type == omapi_datatype_string))
! {
! // struct lease *new_lease = 0;
!
! if(value->u.buffer.len > sizeof(lease->hardware_addr.hbuf) - 1) return DHCP_R_INVALIDARG;
! if(memcmp(&lease->hardware_addr.hbuf[1],value->u.buffer.value,value->u.buffer.len) == 0) return DHCP_R_UNCHANGED;
! /*
! if(!lease_copy(&new_lease,lease,MDL))
! {
! log_info("unable to copy lease");
! return ISC_R_IOERROR;
! }
! memcpy(&new_lease->hardware_addr.hbuf[1],value->u.buffer.value,value->u.buffer.len);
! new_lease->hardware_addr.hlen = value->u.buffer.len + 1;
! if(supersede_lease(lease,new_lease,1,1,1))
! {
! log_info("changed hw_addr to %s",print_hw_addr(0,value->u.buffer.len,value->u.buffer.value));
! return ISC_R_SUCCESS;
! }
! */
! log_info("changed hw_addr to %s",print_hw_addr(0,value->u.buffer.len,value->u.buffer.value));
! dissociate_lease(lease);
! return ISC_R_SUCCESS;
! //log_info("lease hardware change failed.");
! //return ISC_R_IOERROR;
! }
! return DHCP_R_UNCHANGED; /* XXX take change. */
} else if (!omapi_ds_strcmp (name, "hardware-type")) {
return DHCP_R_UNCHANGED; /* XXX take change. */
} else if (lease -> scope) {
--- 293,299 ----
} else if (!omapi_ds_strcmp (name, "billing-class")) {
return DHCP_R_UNCHANGED; /* XXX carefully allow change. */
} else if (!omapi_ds_strcmp (name, "hardware-address")) {
! return DHCP_R_UNCHANGED; /* XXX take change. */
} else if (!omapi_ds_strcmp (name, "hardware-type")) {
return DHCP_R_UNCHANGED; /* XXX take change. */
} else if (lease -> scope) {
***************
*** 1761,1772 ****
return DHCP_R_INVALIDARG;
pool = (struct pool *)h;
! /* Write out a default key and value to avoid the OMAPI client parser bug */
!
! status = omapi_connection_put_name (c,"key_x");
! if(status != ISC_R_SUCCESS) return status;
! status = omapi_connection_put_string (c, "value_x");
! if (status != ISC_R_SUCCESS) return status;
/* Write out the inner object, if any. */
if (h -> inner && h -> inner -> type -> stuff_values) {
--- 1725,1731 ----
return DHCP_R_INVALIDARG;
pool = (struct pool *)h;
! /* Can't stuff pool values yet. */
/* Write out the inner object, if any. */
if (h -> inner && h -> inner -> type -> stuff_values) {
***************
*** 1840,1848 ****
class_dereference(&class->superclass, MDL);
class_reference(&class->superclass, superclass, MDL);
-
- if(class->group) group_dereference(&class->group,MDL);
- clone_group(&class->group,class->superclass->group,MDL);
} else if (value -> type == omapi_datatype_data ||
value -> type == omapi_datatype_string) {
class->name = dmalloc(value->u.buffer.len + 1, MDL);
--- 1799,1804 ----
***************
*** 2001,2008 ****
return DHCP_R_INVALIDARG;
class = (struct class *)h;
- log_info("destroying a class (but doing nothing)");
-
#if defined (DEBUG_MEMORY_LEAKAGE) || \
defined (DEBUG_MEMORY_LEAKAGE_ON_EXIT)
if (class -> nic)
--- 1957,1962 ----
***************
*** 2136,2147 ****
return DHCP_R_INVALIDARG;
class = (struct class *)h;
! /* Write out a default key and value to avoid the OMAPI client parser bug */
!
! status = omapi_connection_put_name (c,"key_x");
! if(status != ISC_R_SUCCESS) return status;
! status = omapi_connection_put_string (c, "value_x");
! if (status != ISC_R_SUCCESS) return status;
/* Write out the inner object, if any. */
if (h -> inner && h -> inner -> type -> stuff_values) {
--- 2090,2096 ----
return DHCP_R_INVALIDARG;
class = (struct class *)h;
! /* Can't stuff class values yet. */
/* Write out the inner object, if any. */
if (h -> inner && h -> inner -> type -> stuff_values) {
***************
*** 2329,2347 ****
struct class *subclass;
isc_result_t status;
! if (h -> type != dhcp_type_subclass)
return DHCP_R_INVALIDARG;
subclass = (struct class *)h;
if (subclass -> name != 0)
return DHCP_R_INVALIDARG;
! /* Write out a default key and value to avoid the OMAPI client parser bug */
!
! status = omapi_connection_put_name (c,"key_x");
! if(status != ISC_R_SUCCESS) return status;
! status = omapi_connection_put_string (c, "value_x");
! if (status != ISC_R_SUCCESS) return status;
/* Write out the inner object, if any. */
if (h -> inner && h -> inner -> type -> stuff_values) {
--- 2278,2291 ----
struct class *subclass;
isc_result_t status;
! if (h -> type != dhcp_type_class)
return DHCP_R_INVALIDARG;
subclass = (struct class *)h;
if (subclass -> name != 0)
return DHCP_R_INVALIDARG;
! /* Can't stuff subclass values yet. */
/* Write out the inner object, if any. */
if (h -> inner && h -> inner -> type -> stuff_values) {
***************
*** 2397,2403 ****
isc_result_t dhcp_subclass_remove (omapi_object_t *lp,
omapi_object_t *id)
{
! #if 0
log_fatal("calling dhcp_subclass_set_value");
/* this should never be called see dhcp_subclass_create for why */
--- 2341,2347 ----
isc_result_t dhcp_subclass_remove (omapi_object_t *lp,
omapi_object_t *id)
{
! #if 1
log_fatal("calling dhcp_subclass_set_value");
/* this should never be called see dhcp_subclass_create for why */
*** mdb.c Wed Dec 28 19:44:54 2011
--- mdb.c.orig Thu Dec 22 15:58:32 2011
***************
*** 440,454 ****
structures, unlike the host objects */
if (commit) {
- if(cp->name) log_info("deleting %s",cp->name);
- else log_info("deleting %s",cp->superclass->name);
- if(cp->billed_leases) log_info("billed leases is non zero");
- if(cp->superclass && cp->superclass->billed_leases) log_info("superclass billed leases is non zero");
write_named_billing_class ((unsigned char *)cp->name, 0, cp);
if (!commit_leases ())
return ISC_R_IOERROR;
}
- if(cp->superclass) class_hash_delete(cp->superclass->hash,(char *)cp->hash_string.data,cp->hash_string.len,MDL);
unlink_class(&cp); /* remove from collections */
--- 440,449 ----
*** confpars.c Thu Dec 15 11:59:18 2011
--- confpars.c.orig Thu Dec 15 11:58:40 2011
***************
*** 2110,2117 ****
/* If we didn't find an existing class, allocate a new one. */
if (!class) {
/* Allocate the class structure... */
- if(type == CLASS_TYPE_CLASS) status = class_allocate (&class, MDL);
- else status = subclass_allocate(&class,MDL);
status = class_allocate (&class, MDL);
if (pc) {
group_reference (&class -> group, pc -> group, MDL);
--- 2110,2115 ----
More information about the dhcp-users
mailing list