Skip to content

Be more defensive in native extension. #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 2, 2019
Merged

Be more defensive in native extension. #10

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
599cdb3
Be more defensive in native extension.
p Oct 25, 2018
e4a0e94
Add some comments
p Oct 25, 2018
7e93999
Review feedback
p Dec 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions ext/mongo_kerberos/mongo_kerberos_native.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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);
Copy link
Contributor

@saghm saghm Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me like it's documented as null-terminated: https://docs.oracle.com/cd/E86824_01/html/E54774/sasl-encode64-3sasl.html

if (is_sasl_failure(result)) {
return Qfalse;
}
Expand All @@ -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;
}
Expand Down