On (25/06/14 23:33), Yassir Elley wrote:
I have attached a revised first patch which addresses the code review
comments. I'm not sure if I needed to (since it has already been ACK'ed),
but I have also attached an unchanged second patch.
Thanks,
Yassir.
I know that Sumit already ACKed these patches, but I used them to test
CI script and I found some minor issues
+ warnings from static analysers (as usual :-)
From b58f39645872bc1dd878ac650861af89964101f9 Mon Sep 17 00:00:00
2001
From: Yassir Elley <yelley(a)redhat.com>
Date: Wed, 21 May 2014 15:11:36 -0400
Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
---
Makefile.am | 22 +
contrib/sssd.spec.in | 2 +-
src/external/samba.m4 | 8 +
src/providers/ad/ad_gpo.c | 887 +++++++++++++++++++++++++++++++++++++++-
src/providers/ad/ad_gpo_child.c | 637 +++++++++++++++++++++++++++++
5 files changed, 1543 insertions(+), 13 deletions(-)
create mode 100644 src/providers/ad/ad_gpo_child.c
diff --git a/Makefile.am b/Makefile.am
index 83999f3584fe130320ebfa6ea459f5706992c6a4..6cf5d8cb0c7c68314dad49461e7cd1fb4452bc1b
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -121,6 +121,9 @@ endif
if BUILD_IFP
sssdlibexec_PROGRAMS += sssd_ifp
endif
+if BUILD_SAMBA
+sssdlibexec_PROGRAMS += gpo_child
+endif
if BUILD_PAC_RESPONDER
@@ -2298,6 +2301,25 @@ ldap_child_LDADD = \
$(DHASH_LIBS) \
$(KRB5_LIBS)
+gpo_child_SOURCES = \
+ src/providers/ad/ad_gpo_child.c \
+ src/util/atomic_io.c \
+ src/util/util.c \
+ src/util/signal.c
+gpo_child_CFLAGS = \
+ $(AM_CFLAGS) \
+ $(POPT_CFLAGS) \
+ $(KRB5_CFLAGS) \
+ $(INI_CONFIG_CFLAGS) \
+ $(SMBCLIENT_CFLAGS)
+gpo_child_LDADD = \
+ libsss_debug.la \
+ $(TALLOC_LIBS) \
+ $(POPT_LIBS) \
+ $(DHASH_LIBS) \
+ $(INI_CONFIG_LIBS) \
+ $(SMBCLIENT_LIBS)
+
proxy_child_SOURCES = \
src/providers/proxy/proxy_child.c \
src/providers/data_provider_iface_generated.c \
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f
100644
--- a/contrib/sssd.spec.in
+++ b/contrib/sssd.spec.in
@@ -97,7 +97,7 @@ BuildRequires: libtdb-devel
BuildRequires: libldb-devel
BuildRequires: libdhash-devel >= 0.4.2
BuildRequires: libcollection-devel
-BuildRequires: libini_config-devel
+BuildRequires: libini_config-devel >= 1.1
BuildRequires: dbus-devel
BuildRequires: dbus-libs
%if 0%{?rhel5_minor} >= 7
diff --git a/src/external/samba.m4 b/src/external/samba.m4
index 735cc5a187dba7514a9c7436a6d3ba948ad522b0..bf790365468da5ccdd40a260d74893490b7e7be8
100644
--- a/src/external/samba.m4
+++ b/src/external/samba.m4
@@ -19,4 +19,12 @@ If you do not want to build these providers it is possible to build
SSSD
without them. In this case, you will need to execute configure script
with argument --without-samba
]]))
+
+ PKG_CHECK_MODULES(INI_CONFIG, ini_config >= 1.0.0, ,
^^^^^
1.1.0
You used correct version in spec file.
configure will pass with old version of ding-libs and compilation fail in
gpo_child.
+ AC_MSG_ERROR([[Please install libini_config development
libraries.
+libini_config libraries are necessary for building ad and ipa provider.
^^^^^^^^^
You touch this code. I might be good to
slightly modify text. "ad with gpo"
or better wording.
Sorry, for nitpicking. I would ignore it there was not problem with version.
+If you do not want to build these providers it is possible to build
SSSD
+without them. In this case, you will need to execute configure script
+with argument --without-samba
+ ]]))
fi
//snip
+/* == ad_gpo_process_cse_send/recv helpers
================================= */
+
+static errno_t
+create_cse_send_buffer(TALLOC_CTX *mem_ctx,
+ char *smb_uri,
+ struct io_buffer **io_buf)
+{
+ struct io_buffer *buf;
+ size_t rp;
+ int smb_uri_length;
+
+ smb_uri_length = strlen(smb_uri);
+
+ DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
+ DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %d\n", smb_uri_length);
+
+ buf = talloc(mem_ctx, struct io_buffer);
+ if (buf == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
+ return ENOMEM;
+ }
+
+ buf->size = 1 * sizeof(uint32_t);
+ if (smb_uri) {
I thing that this test is redundant, because strlen would
crash anyway
with NULL.
Error: REVERSE_INULL
sssd-1.11.92/src/providers/ad/ad_gpo.c:2792: deref_ptr_in_call: Dereferencing pointer
"smb_uri".
sssd-1.11.92/src/providers/ad/ad_gpo.c:2804: check_after_deref: Null-checking
"smb_uri" suggests that it may be null, but it has already been dereferenced on
all paths leading to the check.
+ buf->size += smb_uri_length;
+ }
+
+ DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
+
+ buf->data = talloc_size(buf, buf->size);
+ if (buf->data == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
+ talloc_free(buf);
+ return ENOMEM;
+ }
+
+ rp = 0;
+ /* smb_uri */
+ if (smb_uri) {
+ SAFEALIGN_SET_UINT32(&buf->data[rp], smb_uri_length, &rp);
+ safealign_memcpy(&buf->data[rp], smb_uri, smb_uri_length, &rp);
+ } else {
+ SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
+ }
+
+ *io_buf = buf;
+ return EOK;
+}
I am attaching another html report from clang.
I let final ACK for Sumit.
LS