Re: (ITS#3755) OpenLDAP breaks strict aliasing rules

[h.b.furuseth@usit.uio.no]

Sorry, I forgot that Reply apparently doesn't CC: to openldap-its.
Here is a duplicate of my reply, this time as a followup:


I'm not sure if the patch addresses more than one or two actual
bugs, but the strict aliasing rules make me paranoid enough that
I appreciate any hints the compiler will give.  So I'd like to
eliminate such warnings when that doesn't obscure the code.

As long as we're sure we are not hiding a bug from the compiler
or even introducing one, instead of cleaning the code up...  And
please consider such a warning an invitation to clean up the code
rather than making it uglier.

In any case, as far as I can tell the union trick can leave the
value unspecified.  C99 6.2.6.1p7 says:

  When a value is stored in a member of an object of union type,
  the bytes of the object representation that do not correspond to
  that member but do correspond to other members take unspecified
  values (...)

So while it works with gcc, it might break with other compilers.
There is one exception, from C99 6.5.2.3p5:

  (...) If a union contains several structures that share a common
  initial sequence (see below), and if the union object currently
  contains one of these structures, it is permitted to inspect the
  common initial part of any of them anywhere that a declaration
  of the complete type of the union is visible.  (...)


That only allows your union cookie_u in servers/slapd/acl.c -
assuming AciSetCookie.e is never read after the union has been
updated as a SetCookie, if I understand the above correctly.

That code would be more readable if the declaration was moved
into the if() which creates & uses the cookie.

The clean fix would be to eliminate AciSetCookie and put its old
member e in a new void*data member in SetCookie.  I don't know if
there is any need for that, but I at least can no longer tell if
either the current code or the union-hacked code is correct.


The ldap_pvt_thread_pool_getkey() calls in sl_malloc.c can be
fixed by fetching the key into a variable with the correct type:

@@ ... @@
 {
-	struct slab_heap *sh = NULL;
+	struct slab_heap *sh;
 	ber_len_t size_shift;
@@ ... @@
 #else
+	void *sh_tmp = NULL;
 	ldap_pvt_thread_pool_getkey(
-		ctx, (void *)slap_sl_mem_init, (void **)&sh, NULL );
+		ctx, (void *)slap_sl_mem_init, &sh_tmp, NULL );
+	sh = sh_tmp;
 #endif

The ldap_pvt_thread_pool_setkey() calls need no changes.


BerElementBuffer is not a char array, but BerElementBuffer.buffer
is.  So a simple workaround for that should be to replace
	BerElementBuffer berbuf;
	BerElement *ber = (BerElement *)&berbuf;
with
	BerElementBuffer berbuf;
	BerElement *ber = (BerElement *)berbuf.buffer;
I haven't actually seen any warnings for it, maybe since the
berbuf is not used directly, but maybe you are using a newer gcc.

If someone else feels paranoid we might even give lber_pvt a macro
  /* Convert a BerElementBuffer argument to a BerElement* */
  #define LBER_BUF2ELEMENT(berbuf) ((BerElement *)(berbuf).buffer)
to keep the cast in one place, since the aliasing rules make
casts in general even more suspect than before.

-- 
Hallvard