On Wed, Mar 24, 2010 at 06:56:10AM -0400, Stephen Gallagher wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 03/24/2010 06:49 AM, Stephen Gallagher wrote: > > On 03/23/2010 04:51 PM, Simo Sorce wrote: > >> On Tue, 23 Mar 2010 16:38:57 -0400 > >> Stephen Gallagher <sgall...@redhat.com> wrote: > > > >>> + tempbuf = talloc_realloc(state, state->buf, uint8_t, > >>> + state->len + size); > >>> + if(!tempbuf) { > >>> + tevent_req_error(req, ENOMEM); > >>> return; > >>> } > >>> + state->buf = talloc_steal(state, tempbuf); > > > >> Why the use of tempbuf ? > > > >> you can simply do > >> state->buf = talloc_realloc(state, state->buf, ... > > > >> if state->buf is null you abort the whole operation anyway. > > > >> The talloc_steal is also useless given that talloc_realloc doesn't > >> change the parent. > > > >> Simo. > > > > > > > > You are absolutely correct, of course. New patch behaves better. > > > > I also realized that I had added an extra variable to > > pack_response_packet() in krb5_child.c that was never used (and > > generated a compiler warning). I have removed it. > > > > This time with the patch attached.
NACK. Please consider the following two comments: - can you call free(user_msg) just after do_pam_conversation() to avoid to have it two times? - can you make buf in read_pipe_handler() a buf[MAX_CHILD_MSG_SIZE] or put the read into a while loop to avoid the multiple memory allocation for large messages? Thanks. bye, Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel > > - -- > Stephen Gallagher > RHCE 804006346421761 > > Delivering value year after year. > Red Hat ranks #1 in value among software vendors. > http://www.redhat.com/promo/vendor/ > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.10 (GNU/Linux) > Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ > > iEYEARECAAYFAkup78oACgkQeiVVYja6o6MpVQCfRJaX/AdoZVjgr1J2zAXp3fFa > XLEAnjOgDp2Rshn0bR0x1nSw0CobsI9f > =UB7m > -----END PGP SIGNATURE----- > From 2d5cde2c193234ad7bb254956eaa8fcb63733ac0 Mon Sep 17 00:00:00 2001 > From: Stephen Gallagher <sgall...@redhat.com> > Date: Tue, 23 Mar 2010 16:35:49 -0400 > Subject: [PATCH] Allow arbitrary-length PAM messages > > The PAM standard allows for messages of any length to be returned > to the client. We were discarding all messages of length greater > than 255. This patch dynamically allocates the message buffers so > we can pass the complete message. > > This resolves https://fedorahosted.org/sssd/ticket/432 > --- > src/providers/child_common.c | 27 +++++++++++++++++++-------- > src/providers/child_common.h | 5 ++--- > src/providers/krb5/krb5_auth.c | 2 +- > src/providers/krb5/krb5_child.c | 25 ++++++------------------- > src/providers/ldap/ldap_child.c | 12 ++++++------ > src/sss_client/pam_sss.c | 24 +++++++++++++++++++++--- > src/util/user_info_msg.c | 11 ++++++++--- > 7 files changed, 63 insertions(+), 43 deletions(-) > > diff --git a/src/providers/child_common.c b/src/providers/child_common.c > index 2ad0f04..252b646 100644 > --- a/src/providers/child_common.c > +++ b/src/providers/child_common.c > @@ -149,9 +149,8 @@ struct tevent_req *read_pipe_send(TALLOC_CTX *mem_ctx, > if (req == NULL) return NULL; > > state->fd = fd; > - state->buf = talloc_array(state, uint8_t, MAX_CHILD_MSG_SIZE); > + state->buf = NULL; > state->len = 0; > - if (state->buf == NULL) goto fail; > > fde = tevent_add_fd(ev, state, fd, TEVENT_FD_READ, > read_pipe_handler, req); > @@ -176,6 +175,7 @@ static void read_pipe_handler(struct tevent_context *ev, > struct read_pipe_state); > ssize_t size; > errno_t err; > + uint8_t *buf; > > if (flags & TEVENT_FD_WRITE) { > DEBUG(1, ("read_pipe_done called with TEVENT_FD_WRITE," > @@ -184,9 +184,15 @@ static void read_pipe_handler(struct tevent_context *ev, > return; > } > > + buf = talloc_array(state, uint8_t, CHILD_MSG_CHUNK); > + if (!buf) { > + tevent_req_error(req, ENOMEM); > + return; > + } > + > size = read(state->fd, > - state->buf + state->len, > - MAX_CHILD_MSG_SIZE - state->len); > + buf, > + CHILD_MSG_CHUNK); > if (size == -1) { > err = errno; > if (err == EAGAIN || err == EINTR) { > @@ -198,13 +204,18 @@ static void read_pipe_handler(struct tevent_context *ev, > return; > > } else if (size > 0) { > - state->len += size; > - if (state->len > MAX_CHILD_MSG_SIZE) { > - DEBUG(1, ("read to much, this should never happen.\n")); > - tevent_req_error(req, EINVAL); > + state->buf = talloc_realloc(state, state->buf, uint8_t, > + state->len + size); > + if(!state->buf) { > + tevent_req_error(req, ENOMEM); > return; > } > > + safealign_memcpy(&state->buf[state->len], buf, > + size, &state->len); > + talloc_zfree(buf); > + return; > + > } else if (size == 0) { > DEBUG(6, ("EOF received, client finished\n")); > tevent_req_done(req); > diff --git a/src/providers/child_common.h b/src/providers/child_common.h > index a441df3..0b2081d 100644 > --- a/src/providers/child_common.h > +++ b/src/providers/child_common.h > @@ -33,12 +33,11 @@ > #include "util/util.h" > > #define IN_BUF_SIZE 512 > -#define MAX_CHILD_MSG_SIZE 255 > +#define CHILD_MSG_CHUNK 256 > > struct response { > - size_t max_size; > - size_t size; > uint8_t *buf; > + size_t size; > }; > > struct io_buffer { > diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c > index ce3aacd..880930a 100644 > --- a/src/providers/krb5/krb5_auth.c > +++ b/src/providers/krb5/krb5_auth.c > @@ -1091,7 +1091,7 @@ static void krb5_child_done(struct tevent_req *req) > *msg_len)); > > if ((p + *msg_len) != len) { > - DEBUG(1, ("message format error.\n")); > + DEBUG(1, ("message format error [%d] != [%d].\n", p+*msg_len, len)); > goto done; > } > > diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c > index 86242ef..620e4d1 100644 > --- a/src/providers/krb5/krb5_child.c > +++ b/src/providers/krb5/krb5_child.c > @@ -247,27 +247,15 @@ done: > return kerr; > } > > -static struct response *init_response(TALLOC_CTX *mem_ctx) { > - struct response *r; > - r = talloc(mem_ctx, struct response); > - r->buf = talloc_size(mem_ctx, MAX_CHILD_MSG_SIZE); > - if (r->buf == NULL) { > - DEBUG(1, ("talloc_size failed.\n")); > - return NULL; > - } > - r->max_size = MAX_CHILD_MSG_SIZE; > - r->size = 0; > - > - return r; > -} > - > static errno_t pack_response_packet(struct response *resp, int status, int > type, > size_t len, const uint8_t *data) > { > size_t p = 0; > > - if ((3*sizeof(int32_t) + len +1) > resp->max_size) { > - DEBUG(1, ("response message too big.\n")); > + resp->buf = talloc_array(resp, uint8_t, > + 3*sizeof(int32_t) + len); > + if (!resp->buf) { > + DEBUG(1, ("Insufficient memory to create message.\n")); > return ENOMEM; > } > > @@ -293,9 +281,9 @@ static struct response *prepare_response_message(struct > krb5_req *kr, > size_t user_resp_len; > uint8_t *user_resp; > > - resp = init_response(kr); > + resp = talloc_zero(kr, struct response); > if (resp == NULL) { > - DEBUG(1, ("init_response failed.\n")); > + DEBUG(1, ("Initializing response failed.\n")); > return NULL; > } > > @@ -321,7 +309,6 @@ static struct response *prepare_response_message(struct > krb5_req *kr, > talloc_zfree(msg); > } > } else { > - > if (user_error_message != NULL) { > ret = pack_user_info_chpass_error(kr, user_error_message, > &user_resp_len, &user_resp); > diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c > index 069fbcf..6a78ca0 100644 > --- a/src/providers/ldap/ldap_child.c > +++ b/src/providers/ldap/ldap_child.c > @@ -97,6 +97,11 @@ static int pack_buffer(struct response *r, int result, > const char *msg) > len = strlen(msg); > r->size = 2 * sizeof(uint32_t) + len; > > + r->buf = talloc_array(r, uint8_t, r->size); > + if(!r->buf) { > + return ENOMEM; > + } > + > /* result */ > SAFEALIGN_SET_UINT32(&r->buf[p], result, &p); > > @@ -265,12 +270,7 @@ static int prepare_response(TALLOC_CTX *mem_ctx, > r = talloc_zero(mem_ctx, struct response); > if (!r) return ENOMEM; > > - r->buf = talloc_size(mem_ctx, MAX_CHILD_MSG_SIZE); > - if (r->buf == NULL) { > - DEBUG(1, ("talloc_size failed.\n")); > - return ENOMEM; > - } > - r->max_size = MAX_CHILD_MSG_SIZE; > + r->buf = NULL; > r->size = 0; > > if (kerr == 0) { > diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c > index 07ed4e7..2b1f510 100644 > --- a/src/sss_client/pam_sss.c > +++ b/src/sss_client/pam_sss.c > @@ -588,7 +588,8 @@ static int user_info_chpass_error(pam_handle_t *pamh, > size_t buflen, > { > int ret; > uint32_t msg_len; > - char user_msg[256]; > + char *user_msg; > + size_t bufsize = 0; > > if (buflen < 2* sizeof(uint32_t)) { > D(("User info response data is too short")); > @@ -602,22 +603,39 @@ static int user_info_chpass_error(pam_handle_t *pamh, > size_t buflen, > return PAM_BUF_ERR; > } > > - ret = snprintf(user_msg, sizeof(user_msg), "%s%s%.*s", > + bufsize = strlen(_("Password change failed. ")) + 1; > + > + if (msg_len > 0) { > + bufsize += strlen(_("Server message: ")) + msg_len; > + } > + > + user_msg = (char *)malloc(sizeof(char) * bufsize); > + if (!user_msg) { > + D(("Out of memory.")); > + return PAM_SYSTEM_ERR; > + } > + > + ret = snprintf(user_msg, bufsize, "%s%s%.*s", > _("Password change failed. "), > msg_len > 0 ? _("Server message: ") : "", > msg_len, > msg_len > 0 ? (char *)(buf + 2 * sizeof(uint32_t)) : "" ); > - if (ret < 0 || ret >= sizeof(user_msg)) { > + if (ret < 0 || ret > bufsize) { > D(("snprintf failed.")); > + > + free(user_msg); > return PAM_SYSTEM_ERR; > } > > ret = do_pam_conversation(pamh, PAM_TEXT_INFO, user_msg, NULL, NULL); > if (ret != PAM_SUCCESS) { > D(("do_pam_conversation failed.")); > + > + free(user_msg); > return PAM_SYSTEM_ERR; > } > > + free(user_msg); > return PAM_SUCCESS; > } > > diff --git a/src/util/user_info_msg.c b/src/util/user_info_msg.c > index 547e3bb..505b03d 100644 > --- a/src/util/user_info_msg.c > +++ b/src/util/user_info_msg.c > @@ -33,6 +33,7 @@ errno_t pack_user_info_chpass_error(TALLOC_CTX *mem_ctx, > uint32_t resp_type = SSS_PAM_USER_INFO_CHPASS_ERROR; > size_t err_len; > uint8_t *resp; > + size_t p; > > err_len = strlen(user_error_message); > *resp_len = 2 * sizeof(uint32_t) + err_len; > @@ -42,9 +43,13 @@ errno_t pack_user_info_chpass_error(TALLOC_CTX *mem_ctx, > return ENOMEM; > } > > - memcpy(resp, &resp_type, sizeof(uint32_t)); > - memcpy(resp + sizeof(uint32_t), &err_len, sizeof(uint32_t)); > - memcpy(resp + 2 * sizeof(uint32_t), user_error_message, err_len); > + p = 0; > + SAFEALIGN_SET_UINT32(&resp[p], resp_type, &p); > + SAFEALIGN_SET_UINT32(&resp[p], err_len, &p); > + safealign_memcpy(&resp[p], user_error_message, err_len, &p); > + if (p != *resp_len) { > + DEBUG(0, ("Size mismatch\n")); > + } > > *_resp = resp; > return EOK; > -- > 1.6.6.1 > > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel