Be more defensive in native extension. (#10)

* Be more defensive in native extension.

- Explicitly reserve a byte for null terminator in strings
- Only close the connection being worked on with sasl_dispose rather than all of them

* Add some comments

* Review feedback

Author: p-mongo

ext/mongo_kerberos/mongo_kerberos_native.c

@@ -18,8 +18,14 @@
 
 static void mongo_sasl_conn_free(void* data) {
   sasl_conn_t *conn = (sasl_conn_t*) data;
-  // Ideally we would use sasl_client_done() but that's only available as of cyrus sasl 2.1.25
-  if(conn) sasl_done();
+  if (conn) {
+    sasl_dispose(&conn);
+    /* We do not set connection to NULL in the Ruby object. */
+    /* This is probably fine because this method is supposed to be called */
+    /* when the Ruby object is being garbage collected. */
+    /* Plus, we don't have the Ruby object reference here to do anything */
+    /* with it. */
+  }
 }
 
 static sasl_conn_t* mongo_sasl_context(VALUE self) {
@@ -104,7 +110,14 @@ static VALUE initialize_challenge(VALUE self) {
   }
 
   context = Data_Wrap_Struct(rb_cObject, NULL, mongo_sasl_conn_free, conn);
+  /* I'm guessing ruby raises on out of memory condition rather than */
+  /* returns NULL, hence no error checking is needed here? */
+  
+  /* from now on context owns conn */
+  /* since mongo_sasl_conn_free cleans up conn, we should NOT call */
+  /* sasl_dispose any more in this function. */
   rb_iv_set(self, "@context", context);
+  RB_GC_GUARD(context);
 
   result = sasl_client_start(conn, mechanism_list, NULL, &raw_payload, &raw_payload_len, &mechanism_selected);
   if (is_sasl_failure(result)) {
@@ -115,7 +128,9 @@ static VALUE initialize_challenge(VALUE self) {
     return Qfalse;
   }
 
-  result = sasl_encode64(raw_payload, raw_payload_len, encoded_payload, sizeof(encoded_payload), &encoded_payload_len);
+  /* cyrus-sasl considers `outmax` (fourth argument) to include the null */
+  /* terminator, but this is not documented. Be defensive and exclude it. */
+  result = sasl_encode64(raw_payload, raw_payload_len, encoded_payload, sizeof(encoded_payload)-1, &encoded_payload_len);
   if (is_sasl_failure(result)) {
     return Qfalse;
   }
@@ -135,17 +150,17 @@ static VALUE evaluate_challenge(VALUE self, VALUE rb_payload) {
   step_payload = RSTRING_PTR(rb_payload);
   step_payload_len = (int)RSTRING_LEN(rb_payload);
 
-  result = sasl_decode64(step_payload, step_payload_len, base_payload, sizeof(base_payload), &base_payload_len);
+  result = sasl_decode64(step_payload, step_payload_len, base_payload, sizeof(base_payload)-1, &base_payload_len);
   if (is_sasl_failure(result)) {
     return Qfalse;
   }
 
   result = sasl_client_step(conn, base_payload, base_payload_len, NULL, &out, &outlen);
   if (is_sasl_failure(result)) {
-  	return Qfalse;
+    return Qfalse;
   }
 
-  result = sasl_encode64(out, outlen, payload, sizeof(payload), &payload_len);
+  result = sasl_encode64(out, outlen, payload, sizeof(payload)-1, &payload_len);
   if (is_sasl_failure(result)) {
     return Qfalse;
   }