[SSSD] [PATCH] sss_override: support import and export

Jakub Hrozek jhrozek at redhat.com
Thu Aug 20 11:31:08 UTC 2015


On Thu, Aug 20, 2015 at 12:54:47PM +0200, Pavel Březina wrote:
> On 08/19/2015 11:02 PM, Jakub Hrozek wrote:
> >On Wed, Aug 19, 2015 at 12:46:23PM +0200, Pavel Březina wrote:
> >>On 08/18/2015 05:24 PM, Pavel Březina wrote:
> >>>On 08/16/2015 05:59 PM, Pavel Březina wrote:
> >>>>https://fedorahosted.org/sssd/ticket/2737
> >>>>
> >>>>Hi, it should work... :-) However, I wanted to make import as
> >>>>transaction so no changes are made if some error occurs, but I had some
> >>>>troubles with it so I gave up eventually.
> >>>>
> >>>>Of course, it would be quite difficult to make it safe across domains
> >>>>thus my intention was only to ensure that either all changes are done in
> >>>>a domain or none. But still leaving the possibility that changes are
> >>>>commited in one domain but cancelled in another.
> >>>>
> >>>>I tried to start sysdb transaction on all used domains and then
> >>>>commit/cancel it, writing some neat mechanism for it. However, I
> >>>>occasionally run into a problem when data provider hangs when trying to
> >>>>safe a user. It looked like some sort of race condition.
> >>>>
> >>>>Unfortunately I managed to delete the code so I can't show it to you, I
> >>>>think it would be a nice feature so if anyone familiar with ldb want to
> >>>>step in, he's welcome.
> >>>
> >>>New patches attached.
> >>
> >>I split it into more patches.
> >>
> >
> >There is a strange ordering between this patch and the FQDN -- these
> >patches can't be compiled without the FQDN one, but the FQDN doesn't
> >apply on master.
> 
> Thanks, I fixed that. I'm sending all the patches here including the fqn one
> in correct order.
> 
> >There are still some Coverity errors:
> >Error: COMPILER_WARNING:
> >sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function 'sss_colondb_open'
> >sssd-1.13.1/src/tools/common/sss_colondb.c:273:12: warning: 'fp' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >#     if (fp == NULL && filename != NULL) {
> >#            ^
> >sssd-1.13.1/src/tools/common/sss_colondb.c:259:11: note: 'fp' was declared here
> >#     FILE *fp;
> >#           ^
> >#  271|       }
> >#  272|
> >#  273|->     if (fp == NULL && filename != NULL) {
> >#  274|           ret = errno;
> >#  275|           DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s [%d]: %s\n",
> >
> >Error: NEGATIVE_RETURNS (CWE-394):
> >sssd-1.13.1/src/tools/sss_override.c:340: negative_return_fn: Function "sss_fqname(NULL, 0UL, domain->names, domain, name)" returns a negative number.
> >sssd-1.13.1/src/util/usertools.c:629:41: return_negative_constant: Explicitly returning negative value "-22".
> >sssd-1.13.1/src/tools/sss_override.c:340: var_assign: Assigning: unsigned variable "fqlen" = "sss_fqname".
> >sssd-1.13.1/src/tools/sss_override.c:342: var_tested_neg: Unsigned variable "fqlen" is incremented, which might cause an integer overflow.
> >#  340|       fqlen = sss_fqname(NULL, 0, domain->names, domain, name);
> >#  341|       if (fqlen > 0) {
> >#  342|->         fqlen++; /* \0 */
> >#  343|       } else {
> >#  344|           return NULL;
> 
> Hopefully fixed now.
> 
> >
> >> From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Sat, 15 Aug 2015 13:53:25 +0200
> >>Subject: [PATCH 1/5] sss_override: print input name if unable to parse it
> >
> >ACK
> >
> >
> >> From fcf865ec29f6ef1717a7ca24af6f1bf67ba363a8 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Wed, 19 Aug 2015 12:34:08 +0200
> >>Subject: [PATCH 2/5] TOOLS: add sss_colondb API
> >>
> >>To simplify import/export users and groups.
> >
> >More or less ack :-) see some questions inline
> >
> >>+errno_t sss_colondb_readline(TALLOC_CTX *mem_ctx,
> >>+                             struct sss_colondb *db,
> >>+                             struct sss_colondb_read_field *table)
> >>+{
> >>+    int readchars;
> >>+    size_t linelen = 0;
> >>+    char *line = NULL;
> >>+    char *tcline;
> >>+    char *rest;
> >>+    errno_t ret;
> >>+    int i;
> >>+
> >>+    if (db->mode != SSS_COLONDB_READ) {
> >>+        return ERR_INTERNAL;
> >>+    }
> >>+
> >>+    readchars = getline(&line, &linelen, db->file);
> >>+    if (readchars == -1) {
> >>+        /* Nothing was read. */
> >>+        if (errno != 0) {
> >>+            ret = errno;
> >>+            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read line [%d]: %s",
> >>+                  ret, sss_strerror(ret));
> >>+            return ret;
> >>+        }
> >>+
> >>+        return EOF;
> >>+    }
> >>+
> >>+    /* Copy line to mem_ctx. */
> >>+    tcline = talloc_strdup(mem_ctx, line);
> >
> >Do we need mem_ctx in this function? What about using db (or a special memctx
> >instead of db) instead, then the caller could just talloc_free_children
> >(or we can talloc_free_children even in this function..)
> 
> Yes, it is needed in my opinion. Let us see if we both understand the
> purpose here, if not I'll add a comment.
> 
> We read line with getline() which returns a pointer that needs to be freed.
> But we read data into read_field table and then use it in the caller. The
> problem we need to solve is how to push line pointer to the caller so it can
> be freed when data is no longer needed. Is it clear?
> 
> I can think of three solutions:
> 1) Pass *line as output parameter so the caller can free it.
> 2) Use talloc and talloc_strdup all strings to mem_ctx.
> 3) Use talloc but strdup the whole line to mem_ctx.
> 
> I chose 3) since it contains less allocations and it is quite simple and
> elegant.
> 
> You can't free data in sss_colondb_readline() because you don't know when
> the caller stops using them. And calling talloc_free_children on
> encapsulated db context is not safe. Yes, it is doable now since it doesn't
> contain any children, but the context structure can be changed any time in
> the future.

I would argue that when the caller calls sss_colondb_readline he's done
using the data from the previous line.

> 
> Maybe I don't understand what you mean by "special memctx" since that is
> what I use there.

I meant to have something like:
    sss_colondb_ctx -+
                     +-- readline_ctx

And allocate the line contents internally on readline_ctx.

But I'm not sure if an internal interface is worth all the hassle..

> 
> >
> >>+
> >>+    free(line);
> >>+    line = NULL;
> >
> >> From 0877b501f01dff26a312944a47a98cb88eb47d1e Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Wed, 19 Aug 2015 12:43:15 +0200
> >>Subject: [PATCH 3/5] sss_override: decompose code better
> >
> >ACK
> >
> >thanks for splitting the patch, makes the review much easier.
> >
> >> From 1556d7f744302e8e7e3aa4e3459c5a4c7467bbd9 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Wed, 19 Aug 2015 12:35:12 +0200
> >>Subject: [PATCH 4/5] sss_override: support import and export
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2737
> >
> >Please ask Dan to add the import and export to his tests.
> >
> >Please also ask him to test UPN in the first column -- that's what's the
> >Dell tool supports.
> 
> UPNs won't work yet, I think. I need to finally recreate my AD environment
> before I start working on those.

Please do, do you want me to file a ticket so the effort is tracked? I
haven't tested this scenario either, but the NSS responder supports
lookups by UPNs for some time now...

I'll take a look at the patches..



More information about the sssd-devel mailing list