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 <jhro...@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.
>From b4ffc4165dfc780cc4752df29db632178a316553 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Sun, 20 Oct 2013 19:24:04 +0200
Subject: [PATCH] NSS: use talloc_free_children, check allocation result
Several places in the code didn't check for allocation failures.
Use talloc_free_children instead of talloc_free && talloc_new.
---
src/responder/nss/nsssrv_cmd.c | 24 ++++++++++++++++++++----
src/responder/nss/nsssrv_services.c | 12 ++++++++----
2 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index
d37b4707cb734375011650632bca6d429042038c..67d56f882036b1ddf49832a76355d0a563efb8f6
100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -317,9 +317,15 @@ static int fill_pwent(struct sss_packet *packet,
rp = 2*sizeof(uint32_t);
num = 0;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ i = 0;
+ goto done;
+ }
+
for (i = 0; i < *count; i++) {
- talloc_zfree(tmp_ctx);
- tmp_ctx = talloc_new(NULL);
+ talloc_free_children(tmp_ctx);
msg = msgs[i];
@@ -2313,6 +2319,12 @@ static int fill_grent(struct sss_packet *packet,
num = 0;
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ i = 0;
+ goto done;
+ }
+
/* first 2 fields (len and reserved), filled up later */
ret = sss_packet_grow(packet, 2*sizeof(uint32_t));
if (ret != EOK) {
@@ -2323,8 +2335,8 @@ static int fill_grent(struct sss_packet *packet,
rsize = 0;
for (i = 0; i < *count; i++) {
- talloc_zfree(tmp_ctx);
- tmp_ctx = talloc_new(NULL);
+ talloc_free_children(tmp_ctx);
+
msg = msgs[i];
/* new group */
@@ -3357,6 +3369,10 @@ void nss_update_initgr_memcache(struct nss_ctx *nctx,
}
tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory\n"));
+ goto done;
+ }
ret = sysdb_initgroups(tmp_ctx, dom->sysdb, dom, name, &res);
if (ret != EOK && ret != ENOENT) {
diff --git a/src/responder/nss/nsssrv_services.c
b/src/responder/nss/nsssrv_services.c
index
79caa7d08cdfff25112eedab98a0419f1b1d154e..92e00a723a1141d0a649ff636b2126e820bbc7cc
100644
--- a/src/responder/nss/nsssrv_services.c
+++ b/src/responder/nss/nsssrv_services.c
@@ -625,10 +625,14 @@ fill_service(struct sss_packet *packet,
rzero = 2 * sizeof(uint32_t);
rsize = 0;
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ num = 0;
+ goto done;
+ }
+
for (i = 0; i < msg_count; i++) {
- talloc_zfree(tmp_ctx);
- tmp_ctx = talloc_new(NULL);
- if (!tmp_ctx) return ENOMEM;
+ talloc_free_children(tmp_ctx);
msg = msgs[i];
@@ -762,7 +766,7 @@ fill_service(struct sss_packet *packet,
done:
talloc_free(tmp_ctx);
- if (ret != EOK ||num == 0) {
+ if (ret != EOK || num == 0) {
/* if num is 0 most probably something went wrong,
* reset packet and return ENOENT */
sss_packet_set_size(packet, 0);
--
1.8.3.1
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
