[PATCH] Alignment issues reported by clang.
by Michal Židek
https://fedorahosted.org/sssd/ticket/1359
This patch mostly silences some alignment related warnings reported by
clang, but also fixes some real alignment issues.
But not all warnings are suppressed in this patch. These cases still
generate warnings:
1)=====================================================
src/util/refcount.c:63:25: warning: cast from 'char *' to 'int *'
increases required alignment from 1 to 4 [-Wcast-align]
wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/util/refcount.c:82:25: warning: cast from 'char *' to 'int *'
increases required alignment from 1 to 4 [-Wcast-align]
wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment: This is not a problem, the data we access here is an integer
that is part of a structure, so there should be no alignment issues (C
compiler would not create a structure with inaccessible structure
member). I think suppressing this warning with some additional casts
would only confuse us later and make the code more obfuscated than it is.
2)=====================================================
src/providers/ldap/sdap_async_sudo_hostinfo.c:233:24: warning: cast from
'struct sockaddr *' to 'struct sockaddr_in *' increases
required alignment from 2 to 4 [-Wcast-align]
ip4_addr = (struct sockaddr_in*)(iface->ifa_addr);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/providers/ldap/sdap_async_sudo_hostinfo.c:267:27: warning: cast from
'struct sockaddr *' to 'struct sockaddr_in6 *' increases
required alignment from 2 to 4 [-Wcast-align]
ip6_network = (struct sockaddr_in6*)(iface->ifa_netmask);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...and other sockaddr related casts...
Comment: We only have to make sure that sockaddr is not allocated on
stack here. But we use the getifaddrs() function for this, so it should
be OK.
3)=====================================================
src/sss_client/nss_services.c:137:29: warning: cast from 'char *' to
'char **' increases required alignment from 1 to 8
[-Wcast-align]
sr->result->s_aliases = (char **) &(sr->buffer[i+pad]);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment: Well, there is nothing we can do about this warning. We just
make sure that the accessed data is aligned to sizeof(char **) using the
pad variable (in the original code, we aligned it to 32 bits, but that
was incorrect).
4)=====================================================
src/sss_client/nss_mc_group.c:67:22: warning: cast from 'char *' to
'char **' increases required alignment from 1 to 8 [-Wcast-align]
result->gr_mem = (char **)buffer;
^~~~~~~~~~~~~~~
src/monitor/monitor.c:1588:16: warning: cast from 'char *' to 'struct
inotify_event *' increases required alignment from 1 to 4
[-Wcast-align]
in_event = (struct inotify_event *)buf;
^~~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/nss/nsssrv_mmap_cache.c:281:22: warning: cast from 'char
*' to 'rel_ptr_t *' (aka 'unsigned int *') increases required
alignment from 1 to 4 [-Wcast-align]
name_ptr = *((rel_ptr_t *)rec->data);
^~~~~~~~~~~~~~~~~~~~~~
Comment: Here we access the beginnings of allocated memory spaces. So it
should be OK too. Adding some additional casts just to suppress the
warnings is here IMO inappropriate (it is then not so clear what data
type we use).
5)======================================================
src/responder/common/responder_packet.c:79:21: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->len = &((uint32_t *)packet->buffer)[0];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:80:21: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->cmd = &((uint32_t *)packet->buffer)[1];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:81:24: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->status = &((uint32_t *)packet->buffer)[2];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:82:26: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->reserved = &((uint32_t *)packet->buffer)[3];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:83:33: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:128:29: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->len = &((uint32_t *)packet->buffer)[0];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:129:29: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->cmd = &((uint32_t *)packet->buffer)[1];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:130:32: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->status = &((uint32_t *)packet->buffer)[2];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:131:34: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->reserved = &((uint32_t *)packet->buffer)[3];
^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:132:41: warning: cast from
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
(aka 'unsigned int *') increases required alignment from 1 to 4
[-Wcast-align]
packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
^~~~~~~~~~~~~~~~~~~~~~~~~~
Comment: Same as number 4, but we also use addresses that are multiples
of the sizeof(data_type_used) (relative to the beginning of the buffer).
Which is OK.
================================================================
The rest of the warnings are suppressed by this patch.
I also have one question. Does talloc guarantee that the allocated block
of memory (without the talloc related metadata) is aligned to the
sizeof(biggest_primitive_C_data_type)? I know malloc does this, but I am
not sure about talloc. I saw some places in the code, where we rely on
this feature, so my guess is that talloc does it. Can someone confirm
it? The above mentioned 3, 4 and 5 also rely on proper aligned memory block.
The patch is in attachment.
Thanks
Michal
11 years, 1 month
Design Discussion: SSSD should support DNS sites
by Sumit Bose
Hi,
I have created a design page for
https://fedorahosted.org/sssd/ticket/1032 "[RFE] sssd should support DNS
sites" at
https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryDNSSites .
It can be found below as well.
Corrections, comments and enhancements are welcome.
bye,
Sumit
== Use Active Directory's DNS sites ==
Related ticket(s):
* [https://fedorahosted.org/sssd/ticket/1032 RFE sssd should support DNS sites]
=== Problem Statement ===
In larger Active Directory environments there is typically more than one domain controller. Some of them are used for redundancy, others to build different administrative domains. But in environments with multiple physical locations each location often has at least one local domain controller to reduce latency and network load between the locations.
Now clients have to find the local or nearest domain controller. For this the concept of sites was introduce where each physical location can be seen as an individual site with a unique name. The naming scheme for DNS service records was extended (see e.g. http://technet.microsoft.com/en-us/library/cc759550(v=ws.10).aspx) so that clients can first try to find the needed service in the local site and can fall back to look in the whole domain if there is no local service available.
Additionally clients have to find out about which site they belong to. This must be done dynamically because clients might move from one location to a different one on regular basis (roaming users). For this a special LDAP request, the (C)LDAP ping (http://msdn.microsoft.com/en-us/library/cc223811.aspx), was introduced.
=== Overview view of the solution ===
A new request should be added to the sssd resolver code which can do site aware lookups of DNS service records. This request will do the following steps:
1. do a DNS lookup to find any DC
1. send a CLDAP ping to the returned DC to get the client's site
1. lookup the requested service in the client's site
1. if this fails, lookup the requested service in the next nearest site if available (only available in domains with functional level 2008 or higher)
1. if this fails, lookup the requested service in the 'Default-First-Site-Name' site if it wasn't one of the previously used sites
1. if this fails, lookup the requested service without any site extension
1. if this fails the request will ultimately fail
The results of the different step should be available with one of the debug levels reserved for tracing to make debugging easier and to allow acceptance tests to validate the behavior with the help of the debug logs.
=== Implementation details ===
Currently the request to look up service records if defined by
{{{
struct tevent_req *resolv_getsrv_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
struct resolv_ctx *ctx,
const char *query);
int resolv_getsrv_recv(TALLOC_CTX *mem_ctx,
struct tevent_req *req,
int *status,
int *timeouts,
struct ares_srv_reply **reply_list);
}}}
from async_resolv.h.
The new request might have the following interface:
{{{
/**
* @brief Create an async request to lookup DNS service records and respect the AD DNS sites
*
* @param[in] mem_ctx Talloc memory context for the returned tevent_req structure
* @param[in] ev Tevent context
* @param[in] ctx Resolver context
* @param[in] site_ctx Site related context, may be NULL. If site_ctx is NULL resolv_getsrv_site_send() tries to resolve the site the client belongs to first and then tries to find the nearest server for the requested service.
* @param[in] service the service which should be found, e.g. _ldap._tcp
* @param[in] domain the domain where the service should be found
*
* @return
* - tevent_req structure for the new request
* - NULL if a new request could not be allocated
*/
struct tevent_req *resolv_getsrv_site_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
struct resolv_ctx *ctx,
struct site_ctx *site_ctx,
const char *service,
const char *domain);
/**
* @brief Return the results of the given resolv_getsrv_site request
*
* @param[in] mem_ctx Talloc memory context for the returned results
* @param[in] req resolv_getsrv_site request which should be checked
* @param[out] status the returned status of the underlying c-ares request, see area_query(3) for details
* @param[out] timeouts the number of timeout occured during the underlying c-ares request, see area_query(3) for details
* @param[out] site_ctx an opaque struct with site information which can be used in later requests to skip the site lookup
* @param[out] reply_list list of replies
*
* @return
* - 0 (EOK) on success
* - !=0 in case of an error
*/
int resolv_getsrv_site_recv(TALLOC_CTX *mem_ctx,
struct tevent_req *req,
int *status,
int *timeouts,
struct site_ctx **site_ctx,
struct ares_srv_reply **reply_list);
}}}
==== Finding a DC for the CLDAP ping ====
To find any DC in the domain a Windows client, and samba as well, look for a _ldap._tcp service record. The only difference is that samba uses a plain _ldap._tcp.domain.name while a Windows client _ldap._tcp.Default-First-Site-Name._sites.dc._msdcs.domain.name. I guess it will fall back to _ldap._tcp.domain.name if the other name cannot be resolved. I would suggest to use _ldap._tcp.domain.name for the SSSD implementation.
==== Sending the CLDAP ping ====
The CLDAP ping is a LDAP search request with a filter like
{{{
(&(&(DnsDomain=ad.domain)(DomainSid=S-1-5-21-1111-2222-3333))(NtVer=0x01000016))
}}}
and the attribute "!NetLogon".
The flags given with the !NtVer component of the search filter will be different for a domain member (AD provider) and an IPA server in an environment with trusts (IPA provider).
A domain member will belong to a site and the following flags from /usr/include/samba-4.0/gen_ndr/nbt.h should be used 'NETLOGON_NT_VERSION_5 | NETLOGON_NT_VERSION_5EX | NETLOGON_NT_VERSION_IP'. A trusted server does not belong to one of the sites of trusting domain so it can only ask for the closest site with 'NETLOGON_NT_VERSION_5 | NETLOGON_NT_VERSION_5EX | NETLOGON_NT_VERSION_WITH_CLOSEST_SITE'. Maybe NETLOGON_NT_VERSION_WITH_CLOSEST_SITE is useful for a domain member as well if e.g. the services on the local site are not available.
==== Parsing the server response ====
The server response is a single attribute "!NetLogon" which is a binary blob containing multiple NDR encoded values. This value can be decoded with ndr_pull_netlogon_samlogon_response() from the Samba library libndr-nbt.
=== How to test ===
If this feature is tested the following scenarios can be considered:
==== AD domain does not have any sites other than the default 'Default-First-Site-Name' ====
* SSSD should be able to discover the default site 'Default-First-Site-Name'
* SSSD should connect to any DC.
==== AD domain has sites but the local site of the SSSD client has no domain controller ====
* SSSD should be able to discover the local site
* SSSD should connect to a DC from 'Default-First-Site-Name'
==== AD domain has sites and the local site of the SSSD client has a domain controller ====
* SSSD should be able to discover the local site
* SSSD should connect to a DC from the local site
Besides inspection the log files with a high debug level to connection to the domain controller can also be verified with the netstat or ss utilities.
=== Author(s) ===
Sumit Bose <sbose(a)redhat.com>
11 years, 1 month
[PATCH 0/4] [RFC] Introduce SSSD specific error values.
by Simo Sorce
For a long time I felt constrained using just the error values that
are available in system headers, they do not express the full range
of errors we need to express within SSSD.
So this is a RFC patchset that demonstrates how we could about getting
a richer set of error messages for SSSD that should also simplify some
of our most convoluted code when it comes to returning errors.
The first patch just introduces a very simple error number space and 2
basic errors we always missed. ERR_INTERNAL is what we wanted to use in
many places where we currently return EIO or EFAULT for example.
Patch 2 shows how we can dramatically improve error reporting for our
offline caching authentication code with just a few more targeted errors.
The main thing to note here is that now we can safely distinguish from
a real system error about a missing file (ENOENT) and actually missing
account.
Patch 3 is just an example of fixing EIO in a commonly used macro.
Patch 4 is the most complex example of converting a whole subsystem
that is really complex in its error handling.
Even after the recent refactoring patchesi, krb5_child error reporting
remained quite complex as it uses all of errno, krb5 and pam status
error codes.
The result is that we have to track multiple different error variables
and that can easily lead to hard to read code as well as bugs where one
error code does not reflect into others.
With this patch I think we show how we are able to reduce the complexity
of error handling and at the same time return precise information on what
is worng.
NOTE: This patchset depends on my previous patches for refactoring the
krb5_child as well as the patches that kix the krb5_auth style and the
patches that introduce the authtoken structure.
Simo.
Simo Sorce (4):
Add SSSD specific error codes and definitions
Use SSSD specific errors for offline auth
Return ERR_INTERNAL instead of EIO
Cleanup error message handling for krb5 child
Makefile.am | 4 +-
src/db/sysdb_ops.c | 17 +-
src/providers/krb5/krb5_auth.c | 157 +++++++++-------
src/providers/krb5/krb5_child.c | 388 ++++++++++++++++-----------------------
src/tests/auth-tests.c | 6 +-
src/tests/sysdb-tests.c | 12 +-
src/util/auth_utils.h | 22 ++-
src/util/util.h | 10 +-
src/util/util_errors.c | 50 +++++
src/util/util_errors.h | 85 +++++++++
10 files changed, 417 insertions(+), 334 deletions(-)
create mode 100644 src/util/util_errors.c
create mode 100644 src/util/util_errors.h
11 years, 1 month
Refactor krb5_child code
by Simo Sorce
While working on a different refactoring patch I came across the
krb5_child code and found it very hard to read and follow due to the
growth it has gone through in the past.
I couldn't force myself not to refactor it to make it more linear and
easier to follow and read. A bunch of dead stuff that really confused me
initially also got removed.
(I propose this for master only)
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11 years, 1 month
[PATCH] Refactor the SELinux processing in the PAM responder a little
by Jakub Hrozek
I tried to move the whole PAM processing from the provider into the
responder but stumbled upon several things that had to be refactored in
order to make the code right. These two patches are a start, the next
phase should be getting rid of the per-request score.
So far these patches are intended for master only and their review has
lower priority than reviewing any of the patches targeted for 1.9 as
well.
[PATCH 1/2] PAM: Split SELinux processing into a separate module
No functional change, the SELinux processing just began to occupy a
substantial part of the PAM code, so it had to be split into its own
file.
[PATCH 2/2] PAM: reuse existing code for reading selinux config
The PAM processing reimplemented splitting on separator for no good
reason. This patch utilizes the existing code to reduce code
duplication.
11 years, 1 month
[PATCHES] Fix behavior of 1.5 when backend is stopped
by Ondrej Kos
Hi,
in sssd-1-5, when the backend process is stopped until the pings time
out and backend is restarted, the monitor is still trying to ping the
old process, which results in faulty behavior.
attached are three patches backported for the 1-5(rhel5-9) branch which
fix this.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=886165
Ondra
--
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech
phone: +420-532-294-558
cell: +420-736-417-909
ext: 82-62558
loc: 1013 Brno 1 office
irc: okos @ #brno
11 years, 2 months
third wave for #1747
by Simo Sorce
This is the third, and I think final wave for #1747
These patches change the way subdomains are handled, eliminates the
sysdb_subdom structure and changes a number of sysdb_subdomain
functions.
The last patch is a fix for #1766 as well.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11 years, 2 months