On Mon, Mar 07, 2016 at 01:33:38PM +0100, Lukas Slebodnik wrote:
On (07/03/16 12:12), Pavel Březina wrote:
On 03/07/2016 10:14 AM, Lukas Slebodnik wrote:
ehlo,
simple aptch is attached.
When there, can you also talloc_free(attrs) on error? Thanks.
See updated patch
LS
From 7e70c88277d7ae3f7b953af51da4e97917923fda Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Sat, 5 Mar 2016 18:57:22 +0100 Subject: [PATCH] TOOLS: Prevent dereference of null pointer
VAR_CHECK is called with (var, EOK, ...) EOK would be returned in case of "var != EOK" and output argument _attrs would not be initialized. Therefore there could be dereference of null pointer after calling function usermod_build_attrs.
This is just a suggestion to avoid a macro which change the control flow, i.e. call return or goto. If the code of usermod_build_attrs() is modified like:
if (shell) { attr_name = SYSDB_SHELL; ret = sysdb_attrs_add_string(attrs, attr_name, shell); }
if (ret == EOK && home) { attr_name = SYSDB_HOMEDIR; ret = sysdb_attrs_add_string(attrs, attr_name, home); }
.....
if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Could not add attribute [%s] to changeset.\n", attr_name); goto done; }
the macro can be avoided completely without adding the debug message for every attribute.
bye, Sumit
src/tools/sss_sync_ops.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index 5468929b691c6539cdf55f59be3560412e398f21..b2bbe4c74f5969b097af0f82bb00ca7ec35a596c 100644 --- a/src/tools/sss_sync_ops.c +++ b/src/tools/sss_sync_ops.c @@ -40,7 +40,7 @@ #define VAR_CHECK(var, val, attr, msg) do { \ if (var != (val)) { \ DEBUG(SSSDBG_CRIT_FAILURE, msg" attribute: %s\n", attr); \
return val; \
goto done; \ } \} while(0)
@@ -266,7 +266,12 @@ static int usermod_build_attrs(TALLOC_CTX *mem_ctx, }
*_attrs = attrs;
- return EOK;
- ret = EOK;
+done:
- if (ret != EOK) {
talloc_free(attrs);- }
- return ret;
}
/*
2.7.2
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On 03/07/2016 01:33 PM, Lukas Slebodnik wrote:
On (07/03/16 12:12), Pavel Březina wrote:
On 03/07/2016 10:14 AM, Lukas Slebodnik wrote:
ehlo,
simple aptch is attached.
When there, can you also talloc_free(attrs) on error? Thanks.
See updated patch
Some time has passed now so I take it as you won't implement Sumit's suggestion.
Ack to the patch.
On (15/03/16 11:26), Pavel Březina wrote:
On 03/07/2016 01:33 PM, Lukas Slebodnik wrote:
On (07/03/16 12:12), Pavel Březina wrote:
On 03/07/2016 10:14 AM, Lukas Slebodnik wrote:
ehlo,
simple aptch is attached.
When there, can you also talloc_free(attrs) on error? Thanks.
See updated patch
Some time has passed now so I take it as you won't implement Sumit's suggestion.
I will but I have tasks with higher priority :-)
LS
On (15/03/16 12:31), Lukas Slebodnik wrote:
On (15/03/16 11:26), Pavel Březina wrote:
On 03/07/2016 01:33 PM, Lukas Slebodnik wrote:
On (07/03/16 12:12), Pavel Březina wrote:
On 03/07/2016 10:14 AM, Lukas Slebodnik wrote:
ehlo,
simple aptch is attached.
When there, can you also talloc_free(attrs) on error? Thanks.
See updated patch
Some time has passed now so I take it as you won't implement Sumit's suggestion.
I will but I have tasks with higher priority :-)
Updated patch is attached.
LS
On (06/06/16 18:55), Lukas Slebodnik wrote:
On (15/03/16 12:31), Lukas Slebodnik wrote:
On (15/03/16 11:26), Pavel Březina wrote:
On 03/07/2016 01:33 PM, Lukas Slebodnik wrote:
On (07/03/16 12:12), Pavel Březina wrote:
On 03/07/2016 10:14 AM, Lukas Slebodnik wrote:
ehlo,
simple aptch is attached.
When there, can you also talloc_free(attrs) on error? Thanks.
See updated patch
Some time has passed now so I take it as you won't implement Sumit's suggestion.
I will but I have tasks with higher priority :-)
Updated patch is attached.
LS
From e616ea9e8e58d0ed70b56edc338184d783597004 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Mon, 6 Jun 2016 18:15:44 +0200 Subject: [PATCH] TOOLS: Prevent dereference of null pointer
VAR_CHECK is called with (var, EOK, ...) EOK would be returned in case of "var != EOK" and output argument _attrs would not be initialized. Therefore there could be dereference of null pointer after calling function usermod_build_attrs.
src/tools/sss_sync_ops.c | 62 +++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index 5468929b691c6539cdf55f59be3560412e398f21..e47aef37d2b89b28b7ff18555473136bdf7596cf 100644 --- a/src/tools/sss_sync_ops.c +++ b/src/tools/sss_sync_ops.c
- if (lock == DO_UNLOCK) {
- if (ret == EOK && lock == DO_UNLOCK) {
attr_name = SYSDB_DISABLED; /* PAM code checks for 'false' value in SYSDB_DISABLED attribute */ ret = sysdb_attrs_add_string(attrs,
SYSDB_DISABLED,
attr_name, "false");
VAR_CHECK(ret, EOK, SYSDB_DISABLED,"Could not add attribute to changeset\n");
- }
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"Could not add attribute [%s] to changeset.\n", attr_name);
I forgot to return error here.
} *_attrs = attrs;
Upodated patch is attached.
LS
On 06/07/2016 11:56 AM, Lukas Slebodnik wrote:
On (06/06/16 18:55), Lukas Slebodnik wrote:
On (15/03/16 12:31), Lukas Slebodnik wrote:
On (15/03/16 11:26), Pavel Březina wrote:
On 03/07/2016 01:33 PM, Lukas Slebodnik wrote:
On (07/03/16 12:12), Pavel Březina wrote:
On 03/07/2016 10:14 AM, Lukas Slebodnik wrote: > ehlo, > > simple aptch is attached.
When there, can you also talloc_free(attrs) on error? Thanks.
See updated patch
Some time has passed now so I take it as you won't implement Sumit's suggestion.
I will but I have tasks with higher priority :-)
Updated patch is attached.
LS
From e616ea9e8e58d0ed70b56edc338184d783597004 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Mon, 6 Jun 2016 18:15:44 +0200 Subject: [PATCH] TOOLS: Prevent dereference of null pointer
VAR_CHECK is called with (var, EOK, ...) EOK would be returned in case of "var != EOK" and output argument _attrs would not be initialized. Therefore there could be dereference of null pointer after calling function usermod_build_attrs.
src/tools/sss_sync_ops.c | 62 +++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index 5468929b691c6539cdf55f59be3560412e398f21..e47aef37d2b89b28b7ff18555473136bdf7596cf 100644 --- a/src/tools/sss_sync_ops.c +++ b/src/tools/sss_sync_ops.c
- if (lock == DO_UNLOCK) {
- if (ret == EOK && lock == DO_UNLOCK) {
attr_name = SYSDB_DISABLED; /* PAM code checks for 'false' value in SYSDB_DISABLED attribute */ ret = sysdb_attrs_add_string(attrs,
SYSDB_DISABLED,
attr_name, "false");
VAR_CHECK(ret, EOK, SYSDB_DISABLED,"Could not add attribute to changeset\n");
- }
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"Could not add attribute [%s] to changeset.\n", attr_name);I forgot to return error here.
} *_attrs = attrs;Upodated patch is attached.
LS
Ack.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Sumit Bose