[SSSD] [PATCH] GPO: Fix incorrect strerror on GPO access denial

Lukas Slebodnik lslebodn at redhat.com
Tue Jun 23 11:16:30 UTC 2015


On (23/06/15 10:51), Jakub Hrozek wrote:
>On Tue, Jun 23, 2015 at 10:43:31AM +0200, Lukas Slebodnik wrote:
>> On (23/06/15 10:01), Jakub Hrozek wrote:
>> >On Thu, Jun 11, 2015 at 06:20:34PM +0200, Lukas Slebodnik wrote:
>> >> On (11/06/15 10:44), Stephen Gallagher wrote:
>> >> >On Thu, 2015-06-11 at 16:19 +0200, Lukas Slebodnik wrote:
>> >> >> On (11/06/15 09:35), Stephen Gallagher wrote:
>> >> >> > On Thu, 2015-06-11 at 09:19 -0400, Stephen Gallagher wrote:
>> >> >> > > We're attempting to use strerror() to print the result from
>> >> >> > > ad_gpo_access_check(), but that function returns an extended SSSD
>> >> >> > > errno.
>> >> >> > > 
>> >> >> > > This resulted in "Unknown Error" being printed to the logs.
>> >> >> > 
>> >> >> > And now with the patch attached...
>> >> >> 
>> >> >> > From 108c6e9f47560a53d2d044c0399e66528615d73d Mon Sep 17 00:00:00 
>> >> >> > 2001
>> >> >> > From: Stephen Gallagher <sgallagh at redhat.com>
>> >> >> > Date: Thu, 11 Jun 2015 09:17:02 -0400
>> >> >> > Subject: [PATCH] GPO: Fix incorrect strerror on GPO access denial
>> >> >> > 
>> >> >> > We're attempting to use strerror() to print the result from
>> >> >> > ad_gpo_access_check(), but that function returns an extended SSSD 
>> >> >> > errno
>> >> >> > ---
>> >> >> > src/providers/ad/ad_gpo.c | 2 +-
>> >> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >> > 
>> >> >> > diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
>> >> >> > index 
>> >> >> > 4e7afb12a73313ece00c6ffdd24cfad1b73a831d..995bd083b27710814350bef8d1e43b874d5a8a27 
>> >> >> > 100644
>> >> >> > --- a/src/providers/ad/ad_gpo.c
>> >> >> > +++ b/src/providers/ad/ad_gpo.c
>> >> >> > @@ -1446,11 +1446,11 @@ ad_gpo_perform_hbac_processing(TALLOC_CTX 
>> >> >> > *mem_ctx,
>> >> >> >                               deny_size);
>> >> >> > 
>> >> >> >     if (ret != EOK) {
>> >> >> >         DEBUG(SSSDBG_OP_FAILURE,
>> >> >> >               "GPO access check failed: [%d](%s)\n",
>> >> >> > -              ret, strerror(ret));
>> >> >> > +              ret, sss_strerror(ret));
>> >> >> >         goto done;
>> >> >> >     }
>> >> >> 
>> >> >> You can also replace strerror on line 1152 after 
>> >> >> ad_gpo_extract_policy_setting
>> >> >>                                       1178 after 
>> >> >> ad_gpo_extract_policy_setting
>> >> >>                                       1673 after 
>> >> >> sdap_id_op_connect_recv
>> >> >>                                       1995 after 
>> >> >> ad_gpo_filter_gpos_by_cse_guid
>> >> >>                                       2044 after 
>> >> >> sysdb_gpo_delete_gpo_result_object
>> >> >>                                       2136 after 
>> >> >> sysdb_gpo_get_gpo_by_guid
>> >> >>                                       4037 after 
>> >> >> ad_gpo_parse_gpo_child_response
>> >> >> 
>> >> >> But strerror can be replaced on all places with sss_strerror
>> >> >> because in value is not within sssd error code range then strerror is 
>> >> >> called.
>> >> >
>> >> >
>> >> >To be clear, is this a Nack? I was really only trying to address a
>> >> >specific message that was proving to be misleading. If you really want
>> >> >me to replace all instances of strerror(), that's a bigger effort.
>> >> I didn't want to replace on all places. Michal has patch for this IIRC.
>> >> just in the src/providers/ad/ad_gpo.c. I've already provided lines :-)
>> >> 
>> >> Or should we push 6 one-liners :-) (without ACK)
>> >> 
>> >> LS
>> >
>> >I think we can push this one since it's already written, then continue
>> >with Michal's patch (read=ACK)
>> >
>> Michal has different use case.
>> He want t use sss_strerror on all places and fail with
>> compile time error incase of using "strerror"
>> 
>> >btw what is blocking the mass-conversion to sss_strerror?
>> Nothing.
>> Michal just need to finish his job.
>> 
>> If Stephen does not have a time I can update patch.
>> I provided exact lines which need to be changed.
>
>Sure, if you have time, fix the other occurences in GPO atop Stephen's
>patch and I'll push both.
The commit message would be the same for both patches.
I just updated Stephen's patch and added my signoff.

LS
-------------- next part --------------
>From 06c622365f83fbc8b3a541a54e26c51606f60e81 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgallagh at redhat.com>
Date: Thu, 11 Jun 2015 09:17:02 -0400
Subject: [PATCH] GPO: Fix incorrect strerror on GPO access denial

We're attempting to use strerror() to print the result from
ad_gpo_access_check(), but that function returns an extended SSSD errno

Signed-off-by: Lukas Slebodnik <lslebodn at redhat.com>
---
 src/providers/ad/ad_gpo.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 4e7afb12a73313ece00c6ffdd24cfad1b73a831d..974fd04b99709055f25ed2a3b77821b3caec09ad 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1149,7 +1149,7 @@ ad_gpo_store_policy_settings(struct sss_domain_info *domain,
             if (ret != EOK && ret != ENOENT) {
                 DEBUG(SSSDBG_CRIT_FAILURE,
                       "ad_gpo_extract_policy_setting failed for %s [%d][%s]\n",
-                      allow_key, ret, strerror(ret));
+                      allow_key, ret, sss_strerror(ret));
                 goto done;
             } else if (ret != ENOENT) {
                 ret = sysdb_gpo_store_gpo_result_setting(domain,
@@ -1175,7 +1175,7 @@ ad_gpo_store_policy_settings(struct sss_domain_info *domain,
             if (ret != EOK && ret != ENOENT) {
                 DEBUG(SSSDBG_CRIT_FAILURE,
                       "ad_gpo_extract_policy_setting failed for %s [%d][%s]\n",
-                      deny_key, ret, strerror(ret));
+                      deny_key, ret, sss_strerror(ret));
                 goto done;
             } else if (ret != ENOENT) {
                 ret = sysdb_gpo_store_gpo_result_setting(domain,
@@ -1448,7 +1448,7 @@ ad_gpo_perform_hbac_processing(TALLOC_CTX *mem_ctx,
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
               "GPO access check failed: [%d](%s)\n",
-              ret, strerror(ret));
+              ret, sss_strerror(ret));
         goto done;
     }
 
@@ -1670,7 +1670,7 @@ ad_gpo_connect_done(struct tevent_req *subreq)
         if (dp_error != DP_ERR_OFFLINE) {
             DEBUG(SSSDBG_OP_FAILURE,
                   "Failed to connect to AD server: [%d](%s)\n",
-                  ret, strerror(ret));
+                  ret, sss_strerror(ret));
             goto done;
         } else {
             DEBUG(SSSDBG_TRACE_FUNC, "Preparing for offline operation.\n");
@@ -1992,7 +1992,7 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq)
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
               "Unable to filter GPO list by CSE_GUID: [%d](%s)\n",
-               ret, strerror(ret));
+               ret, sss_strerror(ret));
         goto done;
     }
 
@@ -2041,7 +2041,7 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq)
         default:
             DEBUG(SSSDBG_FATAL_FAILURE,
                   "Could not delete GPO Result from cache: [%s]\n",
-                  strerror(ret));
+                  sss_strerror(ret));
             goto done;
         }
     }
@@ -2133,7 +2133,7 @@ ad_gpo_cse_step(struct tevent_req *req)
         cached_gpt_version = -1;
     } else {
         DEBUG(SSSDBG_FATAL_FAILURE, "Could not read GPO from cache: [%s]\n",
-               strerror(ret));
+              sss_strerror(ret));
         return ret;
     }
 
@@ -4034,7 +4034,7 @@ static void gpo_cse_done(struct tevent_req *subreq)
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE,
               "ad_gpo_parse_gpo_child_response failed: [%d][%s]\n",
-              ret, strerror(ret));
+              ret, sss_strerror(ret));
         tevent_req_error(req, ret);
         return;
     } else if (child_result != 0){
-- 
2.4.3



More information about the sssd-devel mailing list