----- 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(a)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(a)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 whyi
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?
It have to be some hidden gcc optimization, because disabling inlining
function parse_ini_file_with_libini will solve problem (either with -O2 or with
removing static modifier). clang does not do this kind of optimization,
because there isn't such warning. I checked code flow few times and
it should never happen. You properly checked return codes (ret != 0).
On the other side, compiler never lies and sssd is built in koji with
gcc and -O2
LS