[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
[PATCH] Convert ldap_auth to new error codes
by Simo Sorce
This patch removes yet another set of custom and parallel error codes
specified in the sdap_result enumeration, and instead uses the new
unified error codes.
This is to be applied on top of the previous patchset that adds SSSD
specific error codes.
I have done minimal testing with my IPA install and it seems to work
fine as far as ldap+SASL/GSSAPI auth goes.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11 years, 1 month
[PATCH] RFC: Unit test for the NSS responder based on the cmocka library
by Jakub Hrozek
Hi,
Short version - attached is a patch proposal for a unit test based on
cmocka. The patches are an example of a complex, yet isolated unit test
and I'd like to get opinions on whether this would be a good way to go.
Long version:
Our check based test suite is OK for developing simple tests for APIs
like sysdb but not really great for testing of more complex interfaces.
In particular, it's hard to simulate certain pieces of functionality
that require interaction with network or the rest of the system such as
LDAP searches or logins, especially in environments like buildsystems.
I think we might want to look at Cmocka to provide this missing functionality
and test complex interfaces in isolation. Cmocka[1] is a fork of Google's
cmockery which is not maintained upstream anymore. Cmocka is maintained
by Andreas Schneider.
I've been playing with cmocka lately and wrote a couple of patches that
add a unit test for the NSS responder, in particular the getpwnam()
function. It was quite easy to simulate the Data Provider and the client
using two concepts:
1) cmocka's mocking feature
Functions that provide external interface, like the dummy DP functions
in my patches, can access a stack of values. The unit test can push some
expected return values onto the stack before launching the test and the
dummy functions would then pop the values and use them as appropriate.
It's quite easy to simulate a Data Provider saving an entry to cache
with call like this (pseudocode):
int unit_test()
{
will_return(sss_dp_get_account_recv, "testuser");
will_return(sss_dp_get_account_recv, 10);
will_return(sss_dp_get_account_recv, 11);
}
int sss_dp_get_account_recv(TALLOC_CTX *mem_ctx, tevent_req *req)
{
char *username = mock();
uid_t uid = mock();
gid_t gid = mock();
sysdb_store_user(username, uid, gid);
}
2) the -wrap feature of ld
It is possible to selectively override functions from modules linked against
to intercept calls using the -wrap feature of ld. In my unit tests I used
this feature to check results of responder search with defined callbacks
instead of returning the results to the nss client. Here is a simple example
of intercepting a call to negative cache to make sure the negative cache
is hit when it's supposed to:
int __wrap_sss_ncache_check_user(struct sss_nc_ctx *ctx, int ttl,
struct sss_domain_info *dom,
const char *name)
{
int ret;
ret = __real_sss_ncache_check_user(ctx, ttl, dom, name);
if (ret == EEXIST) {
nss_test_ctx->ncache_hit = true;
}
return ret;
}
There's a couple of boilerplate functions but once these are in place,
they can be reused to cover different pieces of functionality..the
attached patches test the following requests:
* cached user including assertion that DP was not contacted
* nonexistant user including assertion that on a subsequent lookup, the
request is returned from negative cache
* search for a user that has not been cache prior to the search
* updating an expired user including assertions that the user had been
updated from DP and the updated entry is returned.
The downsides of using cmocka as far as I can see are:
* writing the mocking code can be tedious. On the other hand, the NSS
responder I picked was probably the most complex code I could test
as it communicates with both DP and the client library. Even more
isolated testing (the AD sites feature comes to mind) would be much
easier I think.
* cmocka is not present in RHEL
I'll be glad to hear comments about this unit test proposal. Patches
are attached.
[1] http://cmocka.cryptomilk.org/
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
