[SSSD] [PATCH] AD: Cache gpo version; only download policy files if version changes

Yassir Elley yelley at redhat.com
Wed Jul 23 07:14:26 UTC 2014



----- Original Message -----
> On (22/07/14 03:13), Yassir Elley wrote:
> >> On Sun, Jul 20, 2014 at 05:48:45PM -0400, Yassir Elley wrote:
> >> > Hi,
> >> > 
> >> > The attached patch adds support for gpo version checking and for
> >> > conditional downloading of policy files. In order to get that to work,
> >> > this patch also replaces the deprecated smb functions (e.g. smbc_open,
> >> > smbc_read, etc) I had been using (not knowing they were deprecated) with
> >> > the newer smb functions, such as
> >> > "smbc_getFunctionOpen(smbc_ctx)(smbc_ctx,
> >> > smb_uri, O_RDONLY, 0755);".
> >> > 
> >> > Before this patch, the policy files were being unconditionally
> >> > downloaded
> >> > every time. After this patch, the backend sends the cached_gpt_version
> >> > (from a previous transaction, if any) to the gpo_child. The gpo_child
> >> > downloads the per-GPO GPT.INI file, from which it extracts the
> >> > sysvol_gpt_version. Only if the sysvol_gpt_version is greater than the
> >> > cached_gpt_version (or if there is no cached_gpt_version) does the
> >> > gpo_child download the policy files. The gpo_child returns the
> >> > sysvol_gpt_version to the backend, which stores it in the cache.
> >> > 
> >> > Note that, while a GPO has both a SYSVOL-stored Group Policy Template
> >> > (gpt)
> >> > version, as well as an LDAP-stored Group Policy Container (gpc) version
> >> > (which should be equal), it is best practice to use the gpt version for
> >> > version checking b/c SYSVOL replication is typically much faster than AD
> >> > replication. In other words, the gpt version is quickly up-to-date on
> >> > all
> >> > DCs for a given domain, while the gpc version may be stale for some time
> >> > after a change is made.
> >> > 
> >> > In order to keep the gpo_child as simple as possible, the gpo_child does
> >> > not interact with the sysdb cache at all (only the backend does).
> >> > However,
> >> > the gpo_child does need to parse the GPT.INI file in order to determine
> >> > whether to download the policy files. As such, the gpo_child includes
> >> > GPT.INI file parsing functionality (which is per-GPO, and not
> >> > CSE-specific).
> >> > 
> >> > Note that this patch does not add support for offline mode, which will
> >> > be
> >> > implemented in a subsequent patch.
> >> > 
> >> > Regards,
> >> > Yassir.
> >> 
> >
> >Hi Jakub,
> >
> >I agree with all of your comments so far and have attached revised patches
> >to this email. I split the original patch into two patches, as you
> >suggested, and I addressed your review comments in the revised patches.
> >Thanks for the review! Keep the comments coming! :)
> >
> >Regards,
> >Yassir.
> 
> >From f3f00a78102ad5e389266b28c43d0396caf57592 Mon Sep 17 00:00:00 2001
> >From: Yassir Elley <yelley at redhat.com>
> >Date: Tue, 22 Jul 2014 03:03:03 -0400
> >Subject: [PATCH 2/2] AD-GPO: only download policy files if gpo version
> >changes
> >
> >---
> > src/providers/ad/ad_gpo.c       | 306 +++++++++++++++++++-----------
> > src/providers/ad/ad_gpo_child.c | 404
> > ++++++++++++++++++++++++++++++++--------
> > 2 files changed, 517 insertions(+), 193 deletions(-)
> >
> 
> I  have few comments to the ad_gpo_child
> 
> >diff --git a/src/providers/ad/ad_gpo_child.c
> >b/src/providers/ad/ad_gpo_child.c
> >index
> >22b8b52046b6298f2eeaf425cef7bf0271865fe2..93ae592e591eefe812212cc99ddbec70c4e0aba9
> >100644
> >--- a/src/providers/ad/ad_gpo_child.c
> >+++ b/src/providers/ad/ad_gpo_child.c
> >@@ -27,6 +27,7 @@
> > #include <sys/stat.h>
> > #include <popt.h>
> > #include <libsmbclient.h>
> >+#include <ini_configobj.h>
> > #include <security/pam_modules.h>
> > 
> > #include "util/util.h"
> >@@ -34,14 +35,17 @@
> > #include "providers/dp_backend.h"
> > #include "sss_cli.h"
> > 
> >-#define RIGHTS_SECTION "Privilege Rights"
> >-#define ALLOW_LOGON_LOCALLY "SeInteractiveLogonRight"
> >-#define DENY_LOGON_LOCALLY "SeDenyInteractiveLogonRight"
> > #define SMB_BUFFER_SIZE 65536
> >+#define GPT_INI "/GPT.INI"
> >+#define INI_GENERAL_SECTION "General"
> >+#define GPT_INI_VERSION "Version"
> > 
> > struct input_buffer {
> >-    const char *smb_uri;
> >-    const char *unix_path;
> >+    int cached_gpt_version;
> >+    const char *smb_server;
> >+    const char *smb_share;
> >+    const char *smb_path;
> >+    const char *smb_cse_suffix;
> > };
> > 
> > static errno_t
> >@@ -51,34 +55,62 @@ unpack_buffer(uint8_t *buf,
> > {
> >     size_t p = 0;
> >     uint32_t len;
> >+    uint32_t cached_gpt_version;
> > 
> >-    /* smb_uri size and length */
> >-    SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
> >-
> >-    DEBUG(SSSDBG_TRACE_ALL, "smb_uri size: %d\n", len);
> >+    /* cached_gpt_version */
> >+    SAFEALIGN_COPY_UINT32_CHECK(&cached_gpt_version, buf + p, size, &p);
> >+    DEBUG(SSSDBG_TRACE_FUNC, "cached_gpt_version: %d\n",
> >cached_gpt_version);
> >+    ibuf->cached_gpt_version = cached_gpt_version;
> > 
> >+    /* smb_server */
> >+    SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
> >+    DEBUG(SSSDBG_TRACE_ALL, "smb_server length: %d\n", len);
> >     if (len == 0) {
> >         return EINVAL;
> >     } else {
> >         if ((p + len ) > size) return EINVAL;
> >-        ibuf->smb_uri = talloc_strndup(ibuf, (char *)(buf + p), len);
> >-        if (ibuf->smb_uri == NULL) return ENOMEM;
> >-        DEBUG(SSSDBG_TRACE_ALL, "got smb_uri: %s\n", ibuf->smb_uri);
> >+        ibuf->smb_server = talloc_strndup(ibuf, (char *)(buf + p), len);
> >+        if (ibuf->smb_server == NULL) return ENOMEM;
> >+        DEBUG(SSSDBG_TRACE_ALL, "smb_server: %s\n", ibuf->smb_server);
> >         p += len;
> >     }
> > 
> >-    /* unix_path size and length */
> >+    /* smb_share */
> >     SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
> >+    DEBUG(SSSDBG_TRACE_ALL, "smb_share length: %d\n", len);
> >+    if (len == 0) {
> >+        return EINVAL;
> >+    } else {
> >+        if ((p + len ) > size) return EINVAL;
> >+        ibuf->smb_share = talloc_strndup(ibuf, (char *)(buf + p), len);
> >+        if (ibuf->smb_share == NULL) return ENOMEM;
> >+        DEBUG(SSSDBG_TRACE_ALL, "smb_share: %s\n", ibuf->smb_share);
> >+        p += len;
> >+    }
> > 
> >-    DEBUG(SSSDBG_TRACE_ALL, "unix_path size: %d\n", len);
> >+    /* smb_path */
> >+    SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
> >+    DEBUG(SSSDBG_TRACE_ALL, "smb_path length: %d\n", len);
> >+    if (len == 0) {
> >+        return EINVAL;
> >+    } else {
> >+        if ((p + len ) > size) return EINVAL;
> >+        ibuf->smb_path = talloc_strndup(ibuf, (char *)(buf + p), len);
> >+        if (ibuf->smb_path == NULL) return ENOMEM;
> >+        DEBUG(SSSDBG_TRACE_ALL, "smb_path: %s\n", ibuf->smb_path);
> >+        p += len;
> >+    }
> > 
> >+    /* smb_cse_suffix */
> >+    SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
> >+    DEBUG(SSSDBG_TRACE_ALL, "smb_cse_suffix length: %d\n", len);
> >     if (len == 0) {
> >         return EINVAL;
> >     } else {
> >         if ((p + len ) > size) return EINVAL;
> >-        ibuf->unix_path = talloc_strndup(ibuf, (char *)(buf + p), len);
> >-        if (ibuf->unix_path == NULL) return ENOMEM;
> >-        DEBUG(SSSDBG_TRACE_ALL, "got unix_path: %s\n", ibuf->unix_path);
> >+        ibuf->smb_cse_suffix = talloc_strndup(ibuf, (char *)(buf + p),
> >len);
> >+        if (ibuf->smb_cse_suffix == NULL) return ENOMEM;
> >+        DEBUG(SSSDBG_TRACE_ALL, "smb_cse_suffix: %s\n",
> >ibuf->smb_cse_suffix);
> >         p += len;
> >     }
> > 
> >@@ -88,14 +120,16 @@ unpack_buffer(uint8_t *buf,
> > 
> > static errno_t
> > pack_buffer(struct response *r,
> >+            int sysvol_gpt_version,
> >             int result)
> > {
> >     size_t p = 0;
> > 
> >     /* A buffer with the following structure must be created:
> >+     *   uint32_t sysvol_gpt_version (required)
> >      *   uint32_t status of the request (required)
> >      */
> >-    r->size = sizeof(uint32_t);
> >+    r->size = 2 * sizeof(uint32_t);
> > 
> >     r->buf = talloc_array(r, uint8_t, r->size);
> >     if(r->buf == NULL) {
> >@@ -104,6 +138,9 @@ pack_buffer(struct response *r,
> > 
> >     DEBUG(SSSDBG_TRACE_FUNC, "result [%d]\n", result);
> > 
> >+    /* sysvol_gpt_version */
> >+    SAFEALIGN_SET_UINT32(&r->buf[p], sysvol_gpt_version, &p);
> >+
> >     /* result */
> >     SAFEALIGN_SET_UINT32(&r->buf[p], result, &p);
> > 
> >@@ -112,6 +149,7 @@ pack_buffer(struct response *r,
> > 
> > static errno_t
> > prepare_response(TALLOC_CTX *mem_ctx,
> >+                 int sysvol_gpt_version,
> >                  int result,
> >                  struct response **rsp)
> > {
> >@@ -126,14 +164,14 @@ prepare_response(TALLOC_CTX *mem_ctx,
> >     r->buf = NULL;
> >     r->size = 0;
> > 
> >-    ret = pack_buffer(r, result);
> >+    ret = pack_buffer(r, sysvol_gpt_version, result);
> >     if (ret != EOK) {
> >         DEBUG(SSSDBG_CRIT_FAILURE, "pack_buffer failed\n");
> >         return ret;
> >     }
> > 
> >     *rsp = r;
> >-    DEBUG(SSSDBG_TRACE_FUNC, "r->size: %zu\n", r->size);
> >+    DEBUG(SSSDBG_TRACE_ALL, "r->size: %zu\n", r->size);
> >     return EOK;
> > }
> > 
> >@@ -153,12 +191,12 @@ sssd_krb_get_auth_data_fn(const char * pServer,
> > 
> > /*
> >  * This function prepares the gpo_cache by:
> >- * - parsing the input_unix_path into its component directories
> >+ * - parsing the input_smb_path into its component directories
> >  * - creating each component directory (if it doesn't already exist)
> >  */
> > static errno_t prepare_gpo_cache(TALLOC_CTX *mem_ctx,
> >                                  const char *cache_dir,
> >-                                 const char *input_unix_path)
> >+                                 const char *input_smb_path_with_suffix)
> > {
> >     char *current_dir;
> >     char *ptr;
> >@@ -167,25 +205,27 @@ static errno_t prepare_gpo_cache(TALLOC_CTX *mem_ctx,
> >     int i;
> >     char *first = NULL;
> >     char *last = NULL;
> >-    char *unix_path = NULL;
> >+    char *smb_path_with_suffix = NULL;
> > 
> >-    unix_path = talloc_strdup(mem_ctx, input_unix_path);
> >-    if (unix_path == NULL) {
> >+    smb_path_with_suffix = talloc_strdup(mem_ctx,
> >input_smb_path_with_suffix);
> >+    if (smb_path_with_suffix == NULL) {
> >         return ENOMEM;
> >     }
> > 
> >+    DEBUG(SSSDBG_TRACE_ALL, "smb_path_with_suffix: %s\n",
> >smb_path_with_suffix);
> >+
> >     current_dir = talloc_strdup(mem_ctx, cache_dir);
> >     if (current_dir == NULL) {
> >         return ENOMEM;
> >     }
> > 
> >-    ptr = unix_path + 1;
> >+    ptr = smb_path_with_suffix + 1;
> >     while ((ptr = strchr(ptr, delim))) {
> >         ptr++;
> >         num_dirs++;
> >     }
> > 
> >-    ptr = unix_path + 1;
> >+    ptr = smb_path_with_suffix + 1;
> > 
> >     for (i = 0; i < num_dirs; i++) {
> >         first = ptr;
> >@@ -210,12 +250,16 @@ static errno_t prepare_gpo_cache(TALLOC_CTX *mem_ctx,
> > 
> > /*
> >  * This function stores the input buf to a local file, whose file path
> >- * is constructed by concatenating GPO_CACHE_PATH to the input unix_path.
> >- * Note that the backend will later read the policy file from the same file
> >path.
> >+ * is constructed by concatenating:
> >+ *   GPO_CACHE_PATH,
> >+ *   input smb_path,
> >+ *   input smb_cse_suffix
> >+ * Note that the backend will later read the file from the same file path.
> >  */
> >-static errno_t store_bytes_in_gpo_cache(const char *unix_path,
> >-                                        uint8_t *buf,
> >-                                        int buflen)
> >+static errno_t gpo_cache_store_file(const char *smb_path,
> >+                                    const char *smb_cse_suffix,
> >+                                    uint8_t *buf,
> >+                                    int buflen)
> > {
> >     int ret;
> >     int fd = -1;
> >@@ -223,6 +267,7 @@ static errno_t store_bytes_in_gpo_cache(const char
> >*unix_path,
> >     ssize_t written;
> >     mode_t old_umask;
> >     char *filename = NULL;
> >+    char *smb_path_with_suffix = NULL;
> >     TALLOC_CTX *tmp_ctx = NULL;
> > 
> >     tmp_ctx = talloc_new(NULL);
> >@@ -231,10 +276,18 @@ static errno_t store_bytes_in_gpo_cache(const char
> >*unix_path,
> >         goto done;
> >     }
> > 
> >-    /* create component directories of unix_path, if needed */
> >-    ret = prepare_gpo_cache(tmp_ctx, GPO_CACHE_PATH, unix_path);
> >+    smb_path_with_suffix =
> >+        talloc_asprintf(tmp_ctx, "%s%s", smb_path, smb_cse_suffix);
> >+    if (smb_path_with_suffix == NULL) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
> >+        ret = ENOMEM;
> >+        goto done;
> >+    }
> > 
> >-    filename = talloc_asprintf(tmp_ctx, GPO_CACHE_PATH"%s", unix_path);
> >+    /* create component directories of smb_path, if needed */
> >+    ret = prepare_gpo_cache(tmp_ctx, GPO_CACHE_PATH, smb_path_with_suffix);
>       ^^^
>       return code is ignored. It may cause problems if prepare_gpo_cache
>       fail.

OK

> >+
> >+    filename = talloc_asprintf(tmp_ctx, GPO_CACHE_PATH"%s",
> >smb_path_with_suffix);
> >     tmp_name = talloc_asprintf(tmp_ctx, "%sXXXXXX", filename);
> >     if (tmp_name == NULL) {
> >         DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
> >@@ -303,21 +356,132 @@ static errno_t store_bytes_in_gpo_cache(const char
> >*unix_path,
> >     return ret;
> > }
> > 
> >+static errno_t
> >+parse_ini_file_with_libini(struct ini_cfgobj *ini_config,
> >+                           int *_gpt_version)
> >+{
> >+    int ret = 0;
> >+    struct value_obj *vobj = NULL;
> >+    int gpt_version;
> >+
> >+    ret = ini_get_config_valueobj(INI_GENERAL_SECTION, GPT_INI_VERSION,
> >+                                  ini_config, INI_GET_FIRST_VALUE, &vobj);
> >+    if (ret != 0) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "ini_get_config_valueobj failed [%d][%s]\n", ret,
> >strerror(ret));
> >+        goto done;
> >+    }
> >+    if (vobj == NULL) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: [%s][%s]\n",
> >+              INI_GENERAL_SECTION, GPT_INI_VERSION);
> >+        ret = EINVAL;
> >+        goto done;
> >+    }
> >+
> >+    gpt_version = ini_get_int32_config_value(vobj, 0, -1, &ret);
> >+    if (ret != 0) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "ini_get_int32_config_value failed [%d][%s]\n",
> >+              ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    *_gpt_version = gpt_version;
> >+
> >+    ret = EOK;
> >+
> >+ done:
> >+
> >+    return ret;
> >+}
> >+
> >+/*
> >+ * This function parses the GPT_INI file stored in the gpo_cache, and uses
> >the
> >+ * results to populate the output parameters ...
> >+ */
> >+static errno_t
> >+ad_gpo_parse_ini_file(const char *smb_path,
> >+                      int *_gpt_version)
> >+{
> >+    struct ini_cfgfile *file_ctx = NULL;
> >+    struct ini_cfgobj *ini_config = NULL;
> >+    const char *ini_filename;
> >+    int ret;
> >+    int gpt_version;
> >+    TALLOC_CTX *tmp_ctx = NULL;
> >+
> >+    tmp_ctx = talloc_new(NULL);
> >+    if (tmp_ctx == NULL) {
> >+        ret = ENOMEM;
> >+        goto done;
> >+    }
> >+
> >+    ini_filename = talloc_asprintf(tmp_ctx, GPO_CACHE_PATH"%s%s",
> >+                                   smb_path, GPT_INI);
>       could you test if allocation was successful?

OK. This happens in several other places in ad_gpo_child.c, so I will fix those too.

> >+
> >+    DEBUG(SSSDBG_TRACE_FUNC, "ini_filename:%s\n", ini_filename);
> >+
> >+
> >+    ret = ini_config_create(&ini_config);
> >+    if (ret != 0) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "ini_config_create failed [%d][%s]\n", ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    ret = ini_config_file_open(ini_filename, 0, &file_ctx);
> >+    if (ret != 0) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "ini_config_file_open failed [%d][%s]\n", ret,
> >strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
> >+    if (ret != 0) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "ini_config_parse failed [%d][%s]\n", ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    ret = parse_ini_file_with_libini(ini_config, &gpt_version);
> >+    if (ret != 0) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "parse_ini_file_with_libini failed [%d][%s]\n",
> >+              ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    *_gpt_version = gpt_version;
> >+
> >+ done:
> >+
> >+    if (ret != EOK) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
> >+    }
> >+
> >+    ini_config_file_destroy(file_ctx);
> >+    ini_config_destroy(ini_config);
> >+    talloc_free(tmp_ctx);
> >+    return ret;
> >+}
> >+
> > /*
> >- * This cse-specific function (GP_EXT_GUID_SECURITY) opens an SMB
> >connection,
> >- * retrieves the data referenced by the input smb_uri, and then closes the
> >SMB
> >- * connection. The data is then written to a file in the GPO_CACHE
> >directory,
> >- * to be read by the backend.
> >+ * This function uses the input smb uri components to download a sysvol
> >file
> >+ * (e.g. INI file, policy file, etc) and store it to the GPO_CACHE
> >directory.
> >  */
> > static errno_t
> >-process_security_settings_cse(TALLOC_CTX *mem_ctx,
> >-                              const char *smb_uri,
> >-                              const char *unix_path)
> >+copy_smb_file_to_gpo_cache(SMBCCTX *smbc_ctx,
> >+                           const char *smb_server,
> >+                           const char *smb_share,
> >+                           const char *smb_path,
> >+                           const char *smb_cse_suffix)
> > {
> >-    SMBCCTX *context;
> >+    char *smb_uri = NULL;
> >+    SMBCFILE *file;
> >     int ret = 0;
> >     uint8_t *buf = NULL;
> >     int buflen = 0;
> >+
> >     TALLOC_CTX *tmp_ctx = NULL;
> > 
> >     tmp_ctx = talloc_new(NULL);
> >@@ -325,59 +489,131 @@ process_security_settings_cse(TALLOC_CTX *mem_ctx,
> >         return ENOMEM;
> >     }
> > 
> >-    DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
> >-    DEBUG(SSSDBG_TRACE_ALL, "%s\n", unix_path);
> >+    smb_uri = talloc_asprintf(tmp_ctx, "%s%s%s%s", smb_server,
> >+                              smb_share, smb_path, smb_cse_suffix);
> >+    DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
> > 
> >-    context = smbc_new_context();
> >-    if (context == NULL) {
> >+    errno = 0;
> >+    file = smbc_getFunctionOpen(smbc_ctx)(smbc_ctx, smb_uri, O_RDONLY,
> >0755);
> >+    if (file == NULL) {
> >+        ret = errno;
> >+        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_getFunctionOpen failed
> >[%d][%s]\n",
> >+              ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
> >+
> >+    errno = 0;
> >+    buflen = smbc_getFunctionRead(smbc_ctx)(smbc_ctx, file, buf,
> >SMB_BUFFER_SIZE);
> >+    if(buflen < 0) {
> >+        ret = errno;
> >+        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_getFunctionRead failed
> >[%d][%s]\n",
> >+              ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    DEBUG(SSSDBG_TRACE_ALL, "smb_buflen: %d\n", buflen);
> >+
> >+    ret = gpo_cache_store_file(smb_path, smb_cse_suffix, buf, buflen);
> >+    if (ret != EOK) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "gpo_cache_store_file failed [%d][%s]\n",
> >+              ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+ done:
> >+    talloc_free(tmp_ctx);
> >+    return ret;
> >+}
> >+
> >+
> >+/*
> >+ * Using its smb_uri components and cached_gpt_version inputs, this
> >function
> >+ * does several things:
> >+ * - it downloads the GPT_INI file to GPO_CACHE
> >+ * - it parses the sysvol_gpt_version field from the GPT_INI file
> >+ * - if the sysvol_gpt_version is greater than the cached_gpt_version
> >+ *   - it downloads the policy file to GPO_CACHE
> >+ * - else
> >+ *   - it doesn't retrieve the policy file
> >+ *   - in this case, the backend will use the existing policy file in
> >GPO_CACHE
> >+ * - it returns the sysvol_gpt_version in the _sysvol_gpt_version output
> >param
> >+ *
> >+ * Note that if the cached_gpt_version sent by the backend is -1 (to
> >indicate
> >+ * that no gpt_version has been set in the cache for the corresponding
> >gpo_guid),
> >+ * then the parsed sysvol_gpt_version (which must be at least 0) will be
> >greater
> >+ * than the cached_gpt_version, thereby triggering a fresh download.
> >+ *
> >+ * Note that the backend will later do the following:
> >+ * - backend will save the the sysvol_gpt_version to sysdb cache
> >+ * - backend will read the policy file from the GPO_CACHE
> >+ */
> >+static errno_t
> >+perform_smb_operations(int cached_gpt_version,
> >+                       const char *smb_server,
> >+                       const char *smb_share,
> >+                       const char *smb_path,
> >+                       const char *smb_cse_suffix,
> >+                       int *_sysvol_gpt_version)
> >+{
> >+    SMBCCTX *smbc_ctx;
> >+    int ret = 0;
>       I don't think we need to initialise ret to zero. From defensive point
>       of
>       view it would be better to init with EINVAL. I think zero can be
>       removed.

OK.

> >+    int sysvol_gpt_version;
> >+
> >+    smbc_ctx = smbc_new_context();
> >+    if (smbc_ctx == NULL) {
> >         DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc
> >         context\n");
> >         ret = ENOMEM;
> >         goto done;
> >     }
> > 
> >-    smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
> >-    smbc_setOptionUseKerberos(context, 1);
> >+    smbc_setFunctionAuthData(smbc_ctx, sssd_krb_get_auth_data_fn);
> >+    smbc_setOptionUseKerberos(smbc_ctx, 1);
> > 
> >     /* Initialize the context using the previously specified options */
> >-    if (smbc_init_context(context) == NULL) {
> >+    if (smbc_init_context(smbc_ctx) == NULL) {
> >         DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc context\n");
> >         ret = ENOMEM;
> >         goto done;
> >     }
> > 
> >-    /* Tell the compatibility layer to use this context */
> >-    smbc_set_context(context);
> >-
> >-    int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
> >-    if (remotehandle < 0) {
> >-        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open failed\n");
> >-        ret = EPIPE;
> >-        goto done;
> >-    }
> >-
> >-    buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
> >-    buflen = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
> >-    if(buflen < 0) {
> >-        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
> >-        ret = EPIPE;
> >-        goto done;
> >-    }
> >-
> >-    DEBUG(SSSDBG_CRIT_FAILURE, "buflen: %d\n", buflen);
> >-
> >-    smbc_close(remotehandle);
> >-
> >-    ret = store_bytes_in_gpo_cache(unix_path, buf, buflen);
> >+    /* download ini file */
> >+    ret = copy_smb_file_to_gpo_cache(smbc_ctx, smb_server, smb_share,
> >smb_path,
> >+                                     GPT_INI);
> >     if (ret != EOK) {
> >         DEBUG(SSSDBG_CRIT_FAILURE,
> >-              "store_bytes_in_gpo_cache failed [%d][%s]\n",
> >+              "copy_smb_file_to_gpo_cache failed [%d][%s]\n",
> >               ret, strerror(ret));
> >         goto done;
> >     }
> > 
> >+    ret = ad_gpo_parse_ini_file(smb_path, &sysvol_gpt_version);
> >+    if (ret != EOK) {
> >+        DEBUG(SSSDBG_CRIT_FAILURE,
> >+              "Cannot parse ini file: [%d][%s]\n", ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n",
> >sysvol_gpt_version);
> >+
> >+    if (sysvol_gpt_version > cached_gpt_version) {
> >+        /* download policy file */
> >+        ret = copy_smb_file_to_gpo_cache(smbc_ctx, smb_server, smb_share,
> >+                                         smb_path, smb_cse_suffix);
> >+        if (ret != EOK) {
> >+            DEBUG(SSSDBG_CRIT_FAILURE,
> >+                  "copy_smb_file_to_gpo_cache failed [%d][%s]\n",
> >+                  ret, strerror(ret));
> >+            goto done;
> >+        }
> >+    }
> >+
> >+    *_sysvol_gpt_version = sysvol_gpt_version;
> >+
> >  done:
> >-    smbc_free_context(context, 0);
> >-    talloc_free(tmp_ctx);
> >+    smbc_free_context(smbc_ctx, 0);
> >     return ret;
> > }
> > 
> >@@ -388,6 +624,7 @@ main(int argc, const char *argv[])
> >     poptContext pc;
> >     int debug_fd = -1;
> >     errno_t ret;
> >+    int sysvol_gpt_version;
> >     int result;
> >     TALLOC_CTX *main_ctx = NULL;
> >     uint8_t *buf = NULL;
> >@@ -484,18 +721,21 @@ main(int argc, const char *argv[])
> >         goto fail;
> >     }
> > 
> >-    DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
> >+    DEBUG(SSSDBG_TRACE_FUNC, "performing smb operations\n");
> > 
> >-    result = process_security_settings_cse(main_ctx,
> >-                                           ibuf->smb_uri,
> >-                                           ibuf->unix_path);
> >+    result = perform_smb_operations(ibuf->cached_gpt_version,
> >+                                    ibuf->smb_server,
> >+                                    ibuf->smb_share,
> >+                                    ibuf->smb_path,
> >+                                    ibuf->smb_cse_suffix,
> >+                                    &sysvol_gpt_version);
> >     if (result != EOK) {
> >         DEBUG(SSSDBG_CRIT_FAILURE,
> >-              "process_security_settings_cse failed.[%d][%s].\n",
> >+              "perform_smb_operations failed.[%d][%s].\n",
> >               result, strerror(result));
>       missing goto fail.
> This is a reason of 1st warning.
> src/providers/ad/ad_gpo_child.c:142:34: error: ‘sysvol_gpt_version’ may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
>      SAFEALIGN_SET_UINT32(&r->buf[p], sysvol_gpt_version, &p);
>                                   ^
> src/providers/ad/ad_gpo_child.c:627:9: note: ‘sysvol_gpt_version’ was
> declared here
>      int sysvol_gpt_version;
> 
> >     }
> 
> 
> There is also another gcc warning. It is only visible with -O1 or higher.
> The same warning is with ferao 20/rawhide and rhel6
> 
>   src/providers/ad/ad_gpo_child.c: In function ‘main’:
>   src/providers/ad/ad_gpo_child.c:142:34: error: ‘gpt_version’ may be used
>   uninitialized in this function [-Werror=maybe-uninitialized]
>        SAFEALIGN_SET_UINT32(&r->buf[p], sysvol_gpt_version, &p);
>                                         ^
>   src/providers/ad/ad_gpo_child.c:413:9: note: ‘gpt_version’ was declared
>   here
>        int gpt_version;
>            ^
>   cc1: all warnings being treated as errors
> 
> There are two possibel solutions:
> a) explicit initialisation to unexpected value.
>   --- a/src/providers/ad/ad_gpo_child.c
>   +++ b/src/providers/ad/ad_gpo_child.c
>   @@ -410,7 +410,7 @@ ad_gpo_parse_ini_file(const char *smb_path,
>        struct ini_cfgobj *ini_config = NULL;
>        const char *ini_filename;
>        int ret;
>   -    int gpt_version;
>   +    int gpt_version = -1;
>        TALLOC_CTX *tmp_ctx = NULL;
> 
> b) disable inlining of parse_ini_file_with_libini.
>   --- a/src/providers/ad/ad_gpo_child.c
>   +++ b/src/providers/ad/ad_gpo_child.c
>   @@ -359,7 +359,7 @@ static errno_t gpo_cache_store_file(const char
>   *smb_path,
>        return ret;
>    }
> 
>   -static errno_t
>   +errno_t
>    parse_ini_file_with_libini(struct ini_cfgobj *ini_config,
>                               int *_gpt_version)
>    {
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> 

I will initialize gpt_version to -1, as that seems to remove the warning.

However, I am a little confused. My understanding of why the sysvol_gpt_version warning is occurring is that perform_smb_operations() might fail. Since I don't have a "goto fail" in the error path, prepare_response() might use an uninitialized value. Adding the "goto fail" makes this warning go away (as you mentioned).

However, I don't understand why the gpt_version warning is occuring. Even though gpt_version is uninitialized in ad_gpo_parse_ini_file, the value of gpt_version is only being used if ad_gpo_parse_ini_file returns EOK. By the time gpt_version is used in pack_buffer, it should definitely be initialized, no?

I will send a revised patch later.

Thanks,
Yassir.



More information about the sssd-devel mailing list