On (21/10/13 11:03), Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */-- 1.8.3.1
Similar problem is in the function nss_update_initgr_memcache
LS
On Mon, Oct 21, 2013 at 11:03:25AM +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM;
num = 0; goto done; is used for other errors in fill_grent() maybe we should do the same here?
bye, Sumit
msg = msgs[i]; /* new group */-- 1.8.3.1
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
plain text document attachment (0001-NSS-Check-allocation-result.patch)
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */
Sorry to shot down another small fix, but I think what you want to do here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use simply talloc_free_children(tmp_ctx);
Simo.
On Mon, Oct 21, 2013 at 10:38:19AM -0400, Simo Sorce wrote:
On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
plain text document attachment (0001-NSS-Check-allocation-result.patch)
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */Sorry to shot down another small fix, but I think what you want to do here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use simply talloc_free_children(tmp_ctx);
Simo.
OK, but that wasn't the point of the patch :-) The point was to check result of allocation, which was missing.
I will prepare a new patch.
On Mon, Oct 21, 2013 at 05:33:33PM +0200, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 10:38:19AM -0400, Simo Sorce wrote:
On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
plain text document attachment (0001-NSS-Check-allocation-result.patch)
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */Sorry to shot down another small fix, but I think what you want to do here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use simply talloc_free_children(tmp_ctx);
Simo.
OK, but that wasn't the point of the patch :-) The point was to check result of allocation, which was missing.
I will prepare a new patch.
New patch is attached -- talloc_free_children is used now instead of talloc_free() && talloc_new().
The fill_* functions return gracefully instead of erroring out now (although I don't think this makes too much of a difference, if sssd runs out of memory, it should just care about not crashing and ending the request somehow).
Also nss_update_initgr_memcache is now checked.
On 10/22/2013 12:29 PM, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 05:33:33PM +0200, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 10:38:19AM -0400, Simo Sorce wrote:
On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
plain text document attachment (0001-NSS-Check-allocation-result.patch)
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */Sorry to shot down another small fix, but I think what you want to do here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use simply talloc_free_children(tmp_ctx);
Simo.
OK, but that wasn't the point of the patch :-) The point was to check result of allocation, which was missing.
I will prepare a new patch.
New patch is attached -- talloc_free_children is used now instead of talloc_free() && talloc_new().
The fill_* functions return gracefully instead of erroring out now (although I don't think this makes too much of a difference, if sssd runs out of memory, it should just care about not crashing and ending the request somehow).
Hi, sorry but nack. We should return ENOMEM in fill_* to stay consistent with rest of the code.
These functions already return errno so I really don't get why you chose to do it this way :-)
Also nss_update_initgr_memcache is now checked.
On Thu, Oct 31, 2013 at 10:37:13AM +0100, Pavel Březina wrote:
On 10/22/2013 12:29 PM, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 05:33:33PM +0200, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 10:38:19AM -0400, Simo Sorce wrote:
On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
plain text document attachment (0001-NSS-Check-allocation-result.patch)
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */Sorry to shot down another small fix, but I think what you want to do here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use simply talloc_free_children(tmp_ctx);
Simo.
OK, but that wasn't the point of the patch :-) The point was to check result of allocation, which was missing.
I will prepare a new patch.
New patch is attached -- talloc_free_children is used now instead of talloc_free() && talloc_new().
The fill_* functions return gracefully instead of erroring out now (although I don't think this makes too much of a difference, if sssd runs out of memory, it should just care about not crashing and ending the request somehow).
Hi, sorry but nack. We should return ENOMEM in fill_* to stay consistent with rest of the code.
These functions already return errno so I really don't get why you chose to do it this way :-)
That's exactly the opposite of what Sumit requested earlier. Please make up your mind :)
On Thu, Oct 31, 2013 at 07:51:20PM +0100, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 10:37:13AM +0100, Pavel Březina wrote:
On 10/22/2013 12:29 PM, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 05:33:33PM +0200, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 10:38:19AM -0400, Simo Sorce wrote:
On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
plain text document attachment (0001-NSS-Check-allocation-result.patch)
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */Sorry to shot down another small fix, but I think what you want to do here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use simply talloc_free_children(tmp_ctx);
Simo.
OK, but that wasn't the point of the patch :-) The point was to check result of allocation, which was missing.
I will prepare a new patch.
New patch is attached -- talloc_free_children is used now instead of talloc_free() && talloc_new().
The fill_* functions return gracefully instead of erroring out now (although I don't think this makes too much of a difference, if sssd runs out of memory, it should just care about not crashing and ending the request somehow).
Hi, sorry but nack. We should return ENOMEM in fill_* to stay consistent with rest of the code.
These functions already return errno so I really don't get why you chose to do it this way :-)
That's exactly the opposite of what Sumit requested earlier. Please make up your mind :)
I think Pavel is right that ENOMEM should be returned but I would prefer to use a single exit in the fill_* calls as well.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 10/31/2013 07:51 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 10:37:13AM +0100, Pavel Březina wrote:
On 10/22/2013 12:29 PM, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 05:33:33PM +0200, Jakub Hrozek wrote:
On Mon, Oct 21, 2013 at 10:38:19AM -0400, Simo Sorce wrote:
On Mon, 2013-10-21 at 11:03 +0200, Jakub Hrozek wrote:
Another small bug I found when looking for #1020945
plain text document attachment (0001-NSS-Check-allocation-result.patch)
From b1d04686f085e25f10dde82f1e19c89278883001 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 20 Oct 2013 19:24:04 +0200 Subject: [PATCH] NSS: Check allocation result
src/responder/nss/nsssrv_cmd.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index d37b4707cb734375011650632bca6d429042038c..a1938b2fc8d0a88460027d5f0d400f840fade02b 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -320,6 +320,7 @@ static int fill_pwent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i];@@ -2325,6 +2326,7 @@ static int fill_grent(struct sss_packet *packet, for (i = 0; i < *count; i++) { talloc_zfree(tmp_ctx); tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return ENOMEM; msg = msgs[i]; /* new group */Sorry to shot down another small fix, but I think what you want to do here is not talloc_zfree(tmp_ctx) + talloc_new(NULL), but rather use simply talloc_free_children(tmp_ctx);
Simo.
OK, but that wasn't the point of the patch :-) The point was to check result of allocation, which was missing.
I will prepare a new patch.
New patch is attached -- talloc_free_children is used now instead of talloc_free() && talloc_new().
The fill_* functions return gracefully instead of erroring out now (although I don't think this makes too much of a difference, if sssd runs out of memory, it should just care about not crashing and ending the request somehow).
Hi, sorry but nack. We should return ENOMEM in fill_* to stay consistent with rest of the code.
These functions already return errno so I really don't get why you chose to do it this way :-)
That's exactly the opposite of what Sumit requested earlier. Please make up your mind :)
Further in the code it makes sense not to report an error, since at least some members may have been evaluated correctly. But it is not the case with this allocation error so it should be reported.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Simo Sorce -
Sumit Bose