access to LDAPEntry from outside of LDAPConn#search causes stack overflow

[hsuenaga]

Found in gem ruby-ldap-0.9.16, ruby-2.1.5.

Because LDAPEntry instance is invalidated in LDAPConn#search block, assertion if( ! ptr->msg ) in macro GET_LDAPENTRY_DATA will fail if the macro used outside of the block.

If the assertion is failed, the macro GET_LDAP_ENTRY_DATA tries to call rb_ldap_entry_inspect() to report errors. rb_ldap_entry_inspect() calls rb_ldap_entry_to_hash(), and rb_ldap_entry_to_hash() uses GET_LDAPENTRY_DATA, and .... call stack will be overflowed.

Here is a sample code.

require 'ldap'

dn = "cn=name,dc=your,dc=domain"
scope = LDAP::LDAP_SCOPE_BASE
filter = "(objectClass=mailAccount)"
found = nil

conn = LDAP::Conn.new('127.0.0.1', LDAP::LDAP_PORT)
conn.set_option(LDAP::LDAP_OPT_PROTOCOL_VERSION, 3)
conn.bind('', '', LDAP::LDAP_AUTH_SIMPLE)

conn.search(dn, scope, filter, nil) do |entry|
  found = entry
end
p found

when executing p found, ruby reports stack level too deep (SystemStackError).

Here is my fix. This patch adds VALUE dn and VALUE attr to LDAPEntry, and changes all of public methods to use the VALUE members instead of raw data structures come from libldap. edata->ldap and edata->msg still exist because LDAPConn may access those data.

The assertion is changed to if( TYPE(ptr->dn) != T_STRING ). I think this assertion is broken, but I have no idea. Using Check_LDAPENTRY() when we really access to raw data structures, and remove the assertion from GET_LDAPENTRY_DATA() ???

Thank you,

diff --git a/entry.c b/entry.c
index b119f48..1a76734 100644
--- a/entry.c
+++ b/entry.c
@@ -8,6 +8,58 @@

 VALUE rb_cLDAP_Entry;

+void
+rb_ldap_entry_mark(RB_LDAPENTRY_DATA *edata)
+{
+  rb_gc_mark(edata->dn);
+  rb_gc_mark(edata->attr);
+}
+
+VALUE
+rb_ldap_entry_val(LDAP *ldap, LDAPMessage *msg, char *c_attr)
+{
+  struct berval **bv;
+  VALUE vals;
+  int nvals;
+  int i;
+
+  bv = ldap_get_values_len(ldap, msg, c_attr);
+  if (bv == NULL)
+    return Qnil;
+
+  nvals = ldap_count_values_len(bv);
+  vals = rb_ary_new2(nvals);
+  for (i = 0; i < nvals; i++) {
+    rb_ary_push(vals, rb_tainted_str_new(bv[i]->bv_val, bv[i]->bv_len));
+  }
+  ldap_value_free_len(bv);
+
+  return vals;
+}
+
+VALUE
+rb_ldap_entry_attr(LDAP *ldap, LDAPMessage *msg)
+{
+  VALUE hash = rb_hash_new();
+  BerElement *ber = NULL;
+  char *c_attr;
+
+  for (c_attr = ldap_first_attribute(ldap, msg, &ber);
+    c_attr != NULL;
+    c_attr = ldap_next_attribute(ldap, msg, ber)) {
+    VALUE attr = rb_tainted_str_new2(c_attr);
+    VALUE vals = rb_ldap_entry_val(ldap, msg, c_attr);
+
+    rb_hash_aset(hash, attr, vals);
+    ldap_memfree(c_attr);
+  }
+
+#if !defined(USE_OPENLDAP1)
+  ber_free(ber, 0);
+#endif
+
+  return hash;
+}

 void
 rb_ldap_entry_free (RB_LDAPENTRY_DATA * edata)
@@ -22,10 +74,25 @@ rb_ldap_entry_new (LDAP * ldap, LDAPMessage * msg)
 {
   VALUE val;
   RB_LDAPENTRY_DATA *edata;
+  char *c_dn;
+
   val = Data_Make_Struct (rb_cLDAP_Entry, RB_LDAPENTRY_DATA,
-             0, rb_ldap_entry_free, edata);
+             rb_ldap_entry_mark, rb_ldap_entry_free, edata);
   edata->ldap = ldap;
   edata->msg = msg;
+
+  /* get dn */
+  c_dn = ldap_get_dn(ldap, msg);
+  if (c_dn) {
+    edata->dn = rb_tainted_str_new2(c_dn);
+    ldap_memfree(c_dn);
+  }
+  else {
+    edata->dn = Qnil;
+  }
+
+  /* get attributes */
+  edata->attr = rb_ldap_entry_attr(ldap, msg);
   return val;
 }

@@ -38,23 +105,10 @@ VALUE
 rb_ldap_entry_get_dn (VALUE self)
 {
   RB_LDAPENTRY_DATA *edata;
-  char *cdn;
-  VALUE dn;

   GET_LDAPENTRY_DATA (self, edata);

-  cdn = ldap_get_dn (edata->ldap, edata->msg);
-  if (cdn)
-    {
-      dn = rb_tainted_str_new2 (cdn);
-      ldap_memfree (cdn);
-    }
-  else
-    {
-      dn = Qnil;
-    }
-
-  return dn;
+  return edata->dn;
 }

 /*
@@ -70,34 +124,10 @@ VALUE
 rb_ldap_entry_get_values (VALUE self, VALUE attr)
 {
   RB_LDAPENTRY_DATA *edata;
-  char *c_attr;
-  struct berval **c_vals;
-  int i;
-  int count;
-  VALUE vals;

   GET_LDAPENTRY_DATA (self, edata);
-  c_attr = StringValueCStr (attr);
-
-  c_vals = ldap_get_values_len (edata->ldap, edata->msg, c_attr);
-  if (c_vals)
-    {
-      vals = rb_ary_new ();
-      count = ldap_count_values_len (c_vals);
-      for (i = 0; i < count; i++)
-   {
-     VALUE str;
-     str = rb_tainted_str_new (c_vals[i]->bv_val, c_vals[i]->bv_len);
-     rb_ary_push (vals, str);
-   }
-      ldap_value_free_len (c_vals);
-    }
-  else
-    {
-      vals = Qnil;
-    }

-  return vals;
+  return rb_hash_aref(edata->attr, attr);
 }

 /*
@@ -111,28 +141,16 @@ VALUE
 rb_ldap_entry_get_attributes (VALUE self)
 {
   RB_LDAPENTRY_DATA *edata;
-  VALUE vals;
-  char *attr;
-  BerElement *ber = NULL;
+  VALUE attrs;

   GET_LDAPENTRY_DATA (self, edata);

-  vals = rb_ary_new ();
-  for (attr = ldap_first_attribute (edata->ldap, edata->msg, &ber);
-       attr != NULL;
-       attr = ldap_next_attribute (edata->ldap, edata->msg, ber))
-    {
-      rb_ary_push (vals, rb_tainted_str_new2 (attr));
-      ldap_memfree(attr);
-    }
-
-    #if !defined(USE_OPENLDAP1)
-    if( ber != NULL ){
-      ber_free(ber, 0);
-    }
-    #endif
+  attrs = rb_funcall(edata->attr, rb_intern("keys"), 0);
+  if (TYPE(attrs) != T_ARRAY) {
+    return Qnil;
+  }

-  return vals;
+  return attrs;
 }

 /*
@@ -144,21 +162,14 @@ rb_ldap_entry_get_attributes (VALUE self)
 VALUE
 rb_ldap_entry_to_hash (VALUE self)
 {
-  VALUE attrs = rb_ldap_entry_get_attributes (self);
-  VALUE hash = rb_hash_new ();
-  VALUE attr, vals;
-  int i;
-
-  Check_Type (attrs, T_ARRAY);
-  rb_hash_aset (hash, rb_tainted_str_new2 ("dn"),
-       rb_ary_new3 (1, rb_ldap_entry_get_dn (self)));
-  for (i = 0; i < RARRAY_LEN (attrs); i++)
-    {
-      attr = rb_ary_entry (attrs, i);
-      vals = rb_ldap_entry_get_values (self, attr);
-      rb_hash_aset (hash, attr, vals);
-    }
+  RB_LDAPENTRY_DATA *edata;
+  VALUE hash, ary;

+  GET_LDAPENTRY_DATA (self, edata);
+  hash = rb_hash_dup(edata->attr);
+  ary = rb_ary_new2(1);
+  rb_ary_push(ary, edata->dn);
+  rb_hash_aset(hash, rb_tainted_str_new2("dn"), ary);
   return hash;
 }

diff --git a/rbldap.h b/rbldap.h
index 7ea4143..c5ef346 100644
--- a/rbldap.h
+++ b/rbldap.h
@@ -55,6 +55,8 @@ typedef struct rb_ldapentry_data
 {
   LDAP *ldap;
   LDAPMessage *msg;
+  VALUE dn;
+  VALUE attr;
 } RB_LDAPENTRY_DATA;

 typedef struct rb_ldapmod_data
@@ -161,8 +163,8 @@ VALUE rb_ldap_mod_vals (VALUE);

 #define Check_LDAPENTRY(obj) {\
   RB_LDAPENTRY_DATA *ptr; \
-  Data_Get_Struct(obj, struct rb_ldapmsg_data, ptr); \
-  if( ! ptr->msg ){ \
+  Data_Get_Struct(obj, struct rb_ldapentry_data, ptr); \
+  if( TYPE(ptr->dn) != T_STRING ){ \
     VALUE value = rb_inspect(obj); \
     rb_raise(rb_eLDAP_InvalidEntryError, "%s is not a valid entry", \
         StringValuePtr(value)); \
@@ -171,7 +173,7 @@ VALUE rb_ldap_mod_vals (VALUE);

 #define GET_LDAPENTRY_DATA(obj,ptr) { \
   Data_Get_Struct(obj, struct rb_ldapentry_data, ptr); \
-  if( ! ptr->msg ){ \
+  if( TYPE(ptr->dn) != T_STRING ){ \
     VALUE value = rb_inspect(obj); \
     rb_raise(rb_eLDAP_InvalidEntryError, "%s is not a valid entry", \
         StringValuePtr(value)); \

[bearded]

Hello @hsuenaga, unfortunately I do not have enough knowledge to answer your questions, but I can merge patch if you prepare it as a Pull request.

[hsuenaga]

Hello @bearded, I sent Pull request. I changed usage of macros, use Check_LDAPENTRY for assertion inside native extension, use GET_LDAPENTRY_DATA just for a type cast.

Please check it, thank you!

[bearded]

Hello @hsuenaga, thank you for pull request!

I just released version 0.9.17, please check it out :)

[hsuenaga]

Hello @bearded, I tested the version 0.9.17 from gem repository. It seems working fine.

Thank you!!