----- 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 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.