On 06/15/2016 08:44 AM, Lukas Slebodnik wrote:
> On (14/06/16 15:30), Pavel Březina wrote:
> > On 05/16/2016 02:00 PM, Pavel Březina wrote:
> > > Hi,
> > > the patches are finally ready to be tested and reviewed. It is too huge
> > > to be sent to the list so please checkout my fedorapeople or github repo:
> > >
> > >
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=backend
> > >
https://github.com/pbrezina/sssd/tree/backend
> > >
> > > Subdomain handlers are not yet converted so subdomain support is
> > > disabled, otherwise everything should work although I'm sure you'll
find
> > > some bugs.
> > >
> > > I managed to do some simple tests (initgroups, authentication) with ldap
> > > provider so far and will continue testing and fixing so if you find a
> > > bug make sure you run with the latest version before reporting it please
> > > :-)
> > >
> > > Since the changes touch almost all areas of SSSD I encourage everyone to
> > > run and try. Some handlers were converted quite easily, some took more
> > > handy work. Areas that are most likely to contain some bugs are these
> > > (please give it extra attention):
> > >
> > > - proxy provider
> > > - if group membership changes during initgroups, nss memory cache should
> > > be clear through dbus call
> > > - selinux support
> > > - hbac support
> > > - change password
> > > - password migration (ipa)
> > >
> > > Don't be alarmed with the number of new lines -- there is not that
many
> > > changes. I copied all touched files and suffixed them with _new so sssd
> > > can be compiled and kept working until the latest patches. I also wanted
> > > to keep the original code intact for comparison (it was easier for
> > > development and it may be handy for bug chasing), you can simply use
> > > some diff tool to see the changes. We can squash it in the end.
> > >
> > > When I will be confident that the patches are stable I will do some
> > > clean up and remove content that is no longer needed.
> >
> > Just a quick update: all found bugs were fixed, CI pass, all downstream tests
> > which were tried (I think all but IPA) pass.
>
> 30 errno_t dp_host_handler(struct sbus_request *sbus_req,
> 31 void *dp_cli,
> 32 uint32_t dp_flags,
> 33 const char *name,
> 34 const char *alias)
> 35 {
> 36 struct dp_hostid_data *data;
> 37 const char *key;
> 38
> 39 if (name == NULL) {
> 40 return EINVAL;
> 41 }
> 42
> 43 data = talloc_zero(sbus_req, struct dp_hostid_data);
> 44 if (data == NULL) {
> 45 return ENOMEM;
> 46 }
> 47
> 48 data->name = name;
> 49 data->alias = alias[0] == '\0' ? NULL : alias;
> 50
> 51 key = talloc_asprintf("%s:%s", name, (alias == '\0' ?
"(null)" : alias));
> ^^^
> The 1st argument should be a talloc context
> otherwise it will CRASH.
>
> 52 if (key == NULL) {
> 53 talloc_free(data);
> 54 return ENOMEM;
> 55 }
Thanks fixed.
>
> I can see a gcc warning on my fedora 24
> src/providers/ad/ad_subdomains_new.c: In function ‘ad_subdomains_refresh_root_done’:
> src/providers/ad/ad_subdomains_new.c:575:23: warning: ‘root_attrs’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
> state->root_attrs = root_attrs;
> ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> src/providers/ad/ad_subdomains_new.c:1161:25: note: ‘root_attrs’ was declared here
> struct sysdb_attrs *root_attrs;
> ^~~~~~~~~~
> src/providers/ad/ad_subdomains_new.c:1194:14: warning: ‘root_id_ctx’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
> subreq = ad_get_slave_domain_send(state, state->ev, state->sd_ctx,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> root_attrs, root_id_ctx->ldap_ctx);
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I seen this in Coverity also reported with root_attrs maybe uninitialized. It
seems to me as false positive but maybe I miss something?
> and there are bunch of new warnings from static analyzers.
> Some of them might be caused by your obsession to move code
> here and back :-)
I'm not really thrilled about those since it looks like the static analyzer
went just mad. Almost all of those warning are about this:
ret = some_fn(&output)
if (ret != EOK) {
goto done;
}
other_fn(output);
^ WARNING: output may be uninitialized
I consider this a false positive unless I miss something?
The other things are false positives as well, probably already marked as
such. When we remove the _new suffix it will hopefully also get rid of it.
Thank you for looking into this.
Thank you very much for your had work.
I thing you deserve a huge steak from manager after this refactoring :-)
LS