Fix ldap child memory hierarchy and other issues

The timeout handler was not a child of the request so it could fire even though
the request was already freed.

The code wouldn't use async writes to the children so it could incur in a short
write with no way to detect or recover from it.
Also fixed style of some helper functions to pass explicit paramters instead of
a general structure.
Add common code to do async writes to pipes.
Fixed async write issue for the krb5_child as well.

Fix also sdap_kinit_done(), a return statement was missing and we were mixing
SDAP_AUTH and errno return codes in state->result

Remove usless helper function that just replicates talloc_strndup()

2 files changed, 97 insertions, 63 deletions

diff --git a/server/providers/krb5/krb5_auth.c b/server/providers/krb5/krb5_auth.c
index 71f4b919a..74981b19b 100644
--- a/server/providers/krb5/krb5_auth.c
+++ b/server/providers/krb5/krb5_auth.c
@@ -575,56 +575,86 @@ static errno_t fork_child(struct krb5child_req *kr)
 }
 
 struct handle_child_state {
+    struct tevent_context *ev;
     struct krb5child_req *kr;
-    ssize_t len;
     uint8_t *buf;
+    ssize_t len;
 };
 
+static void handle_child_step(struct tevent_req *subreq);
 static void handle_child_done(struct tevent_req *subreq);
 
 static struct tevent_req *handle_child_send(TALLOC_CTX *mem_ctx,
                                             struct tevent_context *ev,
                                             struct krb5child_req *kr)
 {
-    int ret;
-    struct tevent_req *req;
-    struct tevent_req *subreq;
+    struct tevent_req *req, *subreq;
     struct handle_child_state *state;
     struct io_buffer *buf;
+    int ret;
+
+    req = tevent_req_create(mem_ctx, &state, struct handle_child_state);
+    if (req == NULL) {
+        return NULL;
+    }
+
+    state->ev = ev;
+    state->kr = kr;
+    state->buf = NULL;
+    state->len = 0;
 
     ret = create_send_buffer(kr, &buf);
     if (ret != EOK) {
         DEBUG(1, ("create_send_buffer failed.\n"));
-        return NULL;
+        goto fail;
     }
 
     ret = fork_child(kr);
     if (ret != EOK) {
         DEBUG(1, ("fork_child failed.\n"));
-        return NULL;
+        goto fail;
     }
 
-    ret = write(kr->write_to_child_fd, buf->data, buf->size);
-    close(kr->write_to_child_fd);
-    kr->write_to_child_fd = -1;
-    if (ret == -1) {
-        DEBUG(1, ("write failed [%d][%s].\n", errno, strerror(errno)));
-        return NULL;
+    subreq = write_pipe_send(state, ev, buf->data, buf->size,
+                             kr->write_to_child_fd);
+    if (!subreq) {
+        ret = ENOMEM;
+        goto fail;
     }
+    tevent_req_set_callback(subreq, handle_child_step, req);
 
-    req = tevent_req_create(mem_ctx, &state, struct handle_child_state);
-    if (req == NULL) {
-        return NULL;
+    return req;
+
+fail:
+    tevent_req_error(req, ret);
+    tevent_req_post(req, ev);
+    return req;
+}
+
+static void handle_child_step(struct tevent_req *subreq)
+{
+    struct tevent_req *req = tevent_req_callback_data(subreq,
+                                                      struct tevent_req);
+    struct handle_child_state *state = tevent_req_data(req,
+                                                    struct handle_child_state);
+    int ret;
+
+    ret = write_pipe_recv(subreq);
+    talloc_zfree(subreq);
+    if (ret != EOK) {
+        tevent_req_error(req, ret);
+        return;
     }
 
-    state->kr = kr;
+    close(state->kr->write_to_child_fd);
+    state->kr->write_to_child_fd = -1;
 
-    subreq = read_pipe_send(state, ev, kr->read_from_child_fd);
-    if (tevent_req_nomem(subreq, req)) {
-        return tevent_req_post(req, ev);
+    subreq = read_pipe_send(state, state->ev, state->kr->read_from_child_fd);
+    if (!subreq) {
+        tevent_req_error(req, ENOMEM);
+        return;
     }
     tevent_req_set_callback(subreq, handle_child_done, req);
-    return req;
 }
 
 static void handle_child_done(struct tevent_req *subreq)
@@ -637,14 +667,14 @@ static void handle_child_done(struct tevent_req *subreq)
 
     ret = read_pipe_recv(subreq, state, &state->buf, &state->len);
     talloc_zfree(subreq);
-    talloc_zfree(state->kr->timeout_handler);
-    close(state->kr->read_from_child_fd);
-    state->kr->read_from_child_fd = -1;
     if (ret != EOK) {
         tevent_req_error(req, ret);
         return;
     }
 
+    close(state->kr->read_from_child_fd);
+    state->kr->read_from_child_fd = -1;
+
     tevent_req_done(req);
     return;
 }
@@ -961,6 +991,7 @@ static void krb5_child_done(struct tevent_req *req)
     int dp_err = DP_ERR_FATAL;
 
     ret = handle_child_recv(req, pd, &buf, &len);
+    talloc_zfree(kr->timeout_handler);
     talloc_zfree(req);
     if (ret != EOK) {
         DEBUG(1, ("child failed (%d [%s])\n", ret, strerror(ret)));
diff --git a/server/providers/krb5/krb5_child.c b/server/providers/krb5/krb5_child.c
index 6eb420bce..6b4c7cc0f 100644
--- a/server/providers/krb5/krb5_child.c
+++ b/server/providers/krb5/krb5_child.c
@@ -339,8 +339,9 @@ static struct response *prepare_response_message(struct krb5_req *kr,
 static errno_t sendresponse(int fd, krb5_error_code kerr, int pam_status,
                             struct krb5_req *kr)
 {
-    int ret;
     struct response *resp;
+    size_t written;
+    int ret;
 
     resp = prepare_response_message(kr, kerr, pam_status);
     if (resp == NULL) {
@@ -348,10 +349,18 @@ static errno_t sendresponse(int fd, krb5_error_code kerr, int pam_status,
         return ENOMEM;
     }
 
-    ret = write(fd, resp->buf, resp->size);
-    if (ret == -1) {
-        DEBUG(1, ("write failed [%d][%s].\n", errno, strerror(errno)));
-        return errno;
+    written = 0;
+    while (written < resp->size) {
+        ret = write(fd, resp->buf + written, resp->size - written);
+        if (ret == -1) {
+            if (errno == EAGAIN || errno == EINTR) {
+                continue;
+            }
+            ret = errno;
+            DEBUG(1, ("write failed [%d][%s].\n", ret, strerror(ret)));
+            return ret;
+        }
+        written += ret;
     }
 
     return EOK;
@@ -680,79 +689,71 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size, struct pam_data *pd,
                              char **ccname, char **keytab, uint32_t *validate)
 {
     size_t p = 0;
-    uint32_t *len;
+    uint32_t len;
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
-    pd->cmd = *len;
+    pd->cmd = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
-    pd->pw_uid = *len;
+    pd->pw_uid = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
-    pd->gr_gid = *len;
+    pd->gr_gid = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
-    *validate = *len;
+    *validate = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
+    len = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
-    if ((p + *len ) > size) return EINVAL;
-    pd->upn = (char *) copy_buffer_and_add_zero(pd, buf+p,
-                                                sizeof(char) * (*len));
+    if ((p + len ) > size) return EINVAL;
+    pd->upn = talloc_strndup(pd, (char *)(buf + p), len);
     if (pd->upn == NULL) return ENOMEM;
-    p += *len;
+    p += len;
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
+    len = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
-    if ((p + *len ) > size) return EINVAL;
-    *ccname = (char *) copy_buffer_and_add_zero(pd, buf+p,
-                                                sizeof(char) * (*len));
+    if ((p + len ) > size) return EINVAL;
+    *ccname = talloc_strndup(pd, (char *)(buf + p), len);
     if (*ccname == NULL) return ENOMEM;
-    p += *len;
+    p += len;
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
+    len = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
-    if ((p + *len ) > size) return EINVAL;
-    *keytab = (char *) copy_buffer_and_add_zero(pd, buf+p,
-                                                sizeof(char) * (*len));
+    if ((p + len ) > size) return EINVAL;
+    *keytab = talloc_strndup(pd, (char *)(buf + p), len);
     if (*keytab == NULL) return ENOMEM;
-    p += *len;
+    p += len;
 
     if ((p + sizeof(uint32_t)) > size) return EINVAL;
-    len = ((uint32_t *)(buf+p));
+    len = *((uint32_t *)(buf + p));
     p += sizeof(uint32_t);
 
-    if ((p + *len) > size) return EINVAL;
-    pd->authtok = copy_buffer_and_add_zero(pd, buf+p, sizeof(char) * (*len));
+    if ((p + len) > size) return EINVAL;
+    pd->authtok = (uint8_t *)talloc_strndup(pd, (char *)(buf + p), len);
     if (pd->authtok == NULL) return ENOMEM;
-    pd->authtok_size = *len + 1;
-    p += *len;
+    pd->authtok_size = len + 1;
+    p += len;
 
     if (pd->cmd == SSS_PAM_CHAUTHTOK) {
         if ((p + sizeof(uint32_t)) > size) return EINVAL;
-        len = ((uint32_t *)(buf+p));
+        len = *((uint32_t *)(buf + p));
         p += sizeof(uint32_t);
 
-        if ((p + *len) > size) return EINVAL;
-        pd->newauthtok = copy_buffer_and_add_zero(pd, buf+p,
-                                                  sizeof(char) * (*len));
+        if ((p + len) > size) return EINVAL;
+        pd->newauthtok = (uint8_t *)talloc_strndup(pd, (char *)(buf + p), len);
         if (pd->newauthtok == NULL) return ENOMEM;
-        pd->newauthtok_size = *len + 1;
-        p += *len;
+        pd->newauthtok_size = len + 1;
+        p += len;
     } else {
         pd->newauthtok = NULL;
         pd->newauthtok_size = 0;
@@ -938,6 +939,8 @@ int main(int argc, const char *argv[])
 
     poptFreeContext(pc);
 
+    DEBUG(7, ("krb5_child started.\n"));
+
     pd = talloc(NULL, struct pam_data);
     if (pd == NULL) {
         DEBUG(1, ("malloc failed.\n"));