Re: [SSSD] [PATCHES] Fix assorted minor bugs found by Coverity
On Wed, Jun 09, 2010 at 12:21:15PM -0400, Stephen Gallagher wrote: On 06/09/2010 11:09 AM, Sumit Bose wrote: NACK, with this patch the backend returns with pd-pam_status not set. Please change it to something like: pd-pam_status = PAM_SYSTEM_ERR; ipa_auth_reply(be_req, DP_ERR_FATAL, pd-pam_status); and call pd = talloc_get_type(be_req-req_data, struct pam_data); at the very beginning of ipa_auth(). New version of this patch attached. For the record, in the future we should rethink how this works. I don't like carrying around a magic pam_data value that needs to be updated separately from calling ipa_auth_reply(). It's not obvious, and that makes it unsafe for new developers. -- Stephen Gallagher RHCE 804006346421761 Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ ok ACK to this and to 0002-0008, too. bye, Sumit ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] Fix assorted minor bugs found by Coverity
On 06/10/2010 05:48 AM, Sumit Bose wrote: ok ACK to this and to 0002-0008, too. Pushed all eight to master and sssd-1-2. -- Stephen Gallagher RHCE 804006346421761 Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] Fix assorted minor bugs found by Coverity
On Wed, Jun 09, 2010 at 10:53:55AM -0400, Stephen Gallagher wrote: All patches apply to both master and sssd-1-2. ... From b21eecc180bb62f2783c50ebae30a20940636c8d Mon Sep 17 00:00:00 2001 From: Stephen Gallagher sgall...@redhat.com Date: Wed, 9 Jun 2010 09:44:47 -0400 Subject: [PATCH 1/8] Avoid potential NULL dereference There was no sense in assigning the value to state-pd-pam_status here at all. https://fedorahosted.org/sssd/ticket/506 --- src/providers/ipa/ipa_auth.c |3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/providers/ipa/ipa_auth.c b/src/providers/ipa/ipa_auth.c index 16bb407ea81d9156b202c0e8a97b5b2bdf3f267b..86a03250044ecac164b1c5396466147feed6395b 100644 --- a/src/providers/ipa/ipa_auth.c +++ b/src/providers/ipa/ipa_auth.c @@ -276,8 +276,7 @@ void ipa_auth(struct be_req *be_req) return; fail: -state-pd-pam_status = PAM_SYSTEM_ERR; -ipa_auth_reply(be_req, DP_ERR_FATAL, state-pd-pam_status); +ipa_auth_reply(be_req, DP_ERR_FATAL, PAM_SYSTEM_ERR); } static void ipa_auth_handler_done(struct tevent_req *req) -- 1.7.0.1 NACK, with this patch the backend returns with pd-pam_status not set. Please change it to something like: pd-pam_status = PAM_SYSTEM_ERR; ipa_auth_reply(be_req, DP_ERR_FATAL, pd-pam_status); and call pd = talloc_get_type(be_req-req_data, struct pam_data); at the very beginning of ipa_auth(). bye, Sumit ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] Fix assorted minor bugs found by Coverity
On 06/09/2010 11:09 AM, Sumit Bose wrote:
NACK, with this patch the backend returns with pd-pam_status not set.
Please change it to something like:
pd-pam_status = PAM_SYSTEM_ERR;
ipa_auth_reply(be_req, DP_ERR_FATAL, pd-pam_status);
and call
pd = talloc_get_type(be_req-req_data, struct pam_data);
at the very beginning of ipa_auth().
New version of this patch attached.
For the record, in the future we should rethink how this works. I don't
like carrying around a magic pam_data value that needs to be updated
separately from calling ipa_auth_reply(). It's not obvious, and that
makes it unsafe for new developers.
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
From 56038caea7424c5d9c0f963b453ceb2ec25edca6 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher sgall...@redhat.com
Date: Wed, 9 Jun 2010 09:44:47 -0400
Subject: [PATCH 1/8] Avoid potential NULL dereference
https://fedorahosted.org/sssd/ticket/506
---
src/providers/ipa/ipa_auth.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/providers/ipa/ipa_auth.c b/src/providers/ipa/ipa_auth.c
index 16bb407ea81d9156b202c0e8a97b5b2bdf3f267b..283196dc3de322a7f36b85db724fb89cfcc5bf98 100644
--- a/src/providers/ipa/ipa_auth.c
+++ b/src/providers/ipa/ipa_auth.c
@@ -233,6 +233,7 @@ void ipa_auth(struct be_req *be_req)
{
struct tevent_req *req;
struct ipa_auth_state *state;
+struct pam_data *pd = talloc_get_type(be_req-req_data, struct pam_data);
state = talloc_zero(be_req, struct ipa_auth_state);
if (state == NULL) {
@@ -246,7 +247,7 @@ void ipa_auth(struct be_req *be_req)
state-be_req = be_req;
state-ev = be_req-be_ctx-ev;
-state-pd = talloc_get_type(be_req-req_data, struct pam_data);
+state-pd = pd;
switch (state-pd-cmd) {
case SSS_PAM_AUTHENTICATE:
@@ -276,8 +277,9 @@ void ipa_auth(struct be_req *be_req)
return;
fail:
-state-pd-pam_status = PAM_SYSTEM_ERR;
-ipa_auth_reply(be_req, DP_ERR_FATAL, state-pd-pam_status);
+talloc_free(state);
+pd-pam_status = PAM_SYSTEM_ERR;
+ipa_auth_reply(be_req, DP_ERR_FATAL, pd-pam_status);
}
static void ipa_auth_handler_done(struct tevent_req *req)
--
1.7.0.1
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] Fix assorted minor bugs found by Coverity
On Wed, Jun 09, 2010 at 12:21:15PM -0400, Stephen Gallagher wrote: On 06/09/2010 11:09 AM, Sumit Bose wrote: NACK, with this patch the backend returns with pd-pam_status not set. Please change it to something like: pd-pam_status = PAM_SYSTEM_ERR; ipa_auth_reply(be_req, DP_ERR_FATAL, pd-pam_status); and call pd = talloc_get_type(be_req-req_data, struct pam_data); at the very beginning of ipa_auth(). New version of this patch attached. For the record, in the future we should rethink how this works. I don't like carrying around a magic pam_data value that needs to be updated separately from calling ipa_auth_reply(). It's not obvious, and that makes it unsafe for new developers. yes, that's the idea behind #453. bye, Sumit -- Stephen Gallagher RHCE 804006346421761 Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
