[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] tests: Print warning if LDB_MODULES_PATH is not set
by Michal Židek
"make check" sets the LDB_MODULES_PATH automatically, but if run as
individual tests, the environment variable must be set manually,
otherwise the system memberof.so is used. This patch adds warning if
the default system memberof.so is used in sysdb-tests and
sysdb_ssh-tests.
Patch attached.
Michal
11 years, 1 month
[PATCH] BUILD: Always run distcheck and RPM tests in /dev/shm
by Stephen Gallagher
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Some of the tests (such as the sysdb tests) are highly I/O limited.
By running them on a ramdisk, we can significantly speed up the
test runs when doing a distcheck or RPM build.
The RPM specfile is not conditionalized like the Makefile because it
is highly Fedora/RHEL-centric and is therefore guaranteed to be available.
For non-RPM/distcheck builds, you can always pass
- --with-test-dir=/dev/shm to configure (which is done automatically in
the contrib/fedora/bashrc_sssd script fedconfig macro).
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlFHKT8ACgkQeiVVYja6o6Me9gCdHQWRm+V8gvvGHFuTrEFx19Zx
JVAAn1c15sLVcOoLZxKRZPFsDAtaI14l
=y/z2
-----END PGP SIGNATURE-----
11 years, 1 month
[PATCH] Convert sdap_access code to new error code facility
by Simo Sorce
Not fully tested, but I am reasonably confident it should work just
fine.
The changes to the HBAC code are the ones I am a bit concerned with.
>From my reading of the code, the 'fail' case migh return PAM_SUCCESS
even though HABC failed to operate before the patches, because
pam_success was set when sdap_access_recv() was called.
I changed that code to unconditionally return PAM_SYSTEM_ERR because
I can't see any legitimate case where an error would still warrant
returning success. This is really a corner case that is almost
impossible to hit though. I think the only case where it would return
the wrong result is in case the hbac search base was missing and we
still got this far somehow.
So please check carefully.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11 years, 1 month
[PATCHES] [PUSHED] Look up missing names in the simple access provider
by Jakub Hrozek
Hi,
the attached patches fix CVE-2013-0287. They were reviewed off-list by
Simo, Pavel and Ondra, so I pushed them already a while ago. For posterity,
I'm also sending them to the sssd-devel list. Even retroactive comments
or nacks are appreciated.
The attached patches only apply to master, sssd-1-9 versions will follow
up shortly.
11 years, 1 month