Re: [SSSD] [PATCH] Allow arbitrary-length PAM messages

2010-03-25 Thread Sumit Bose 
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);

Re: [SSSD] [PATCH] Allow arbitrary-length PAM messages

2010-03-25 Thread Stephen Gallagher 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 03/25/2010 09:56 AM, Sumit Bose wrote:
 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
 

New patch attached. Thanks for the review.


- -- 
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/

iEYEARECAAYFAkurcFsACgkQeiVVYja6o6NI5ACcDhFtJ+tyyTMF074mMVOP02Td
Bj4AnR4UxRVUG5IvluJkVr+RCJJXiq6D
=nIKf
-END PGP SIGNATURE-
From de5bd8fed5074715988810238ef2927746c7be8a 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|   20 
 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|   23 ---
 src/util/user_info_msg.c|   11 ---
 7 files changed, 55 insertions(+), 43 deletions(-)

diff --git a/src/providers/child_common.c b/src/providers/child_common.c
index 2ad0f04..b980255 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[CHILD_MSG_CHUNK];
 
 if (flags  TEVENT_FD_WRITE) {
 DEBUG(1, (read_pipe_done called with TEVENT_FD_WRITE,
@@ -185,8 +185,8 @@ static void read_pipe_handler(struct tevent_context *ev,
 }
 
 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 +198,17 @@ 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);
+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

Re: [SSSD] [PATCH] Allow arbitrary-length PAM messages

2010-03-24 Thread Stephen Gallagher 
-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.
___
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

Re: [SSSD] [PATCH] Allow arbitrary-length PAM messages

2010-03-23 Thread Simo Sorce 
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.

-- 
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel