Re: [PATCH 04/19] libctf: low-level list manipulation and helper utilities

[Nick Alcock <nick.alcock@oracle.com>]

On 2 May 2019, Nick Clifton said:

> Hi Nick,
>
>> +#include <gelf.h>
>
> This header is from the elfutils project right ?  Given that, and the fact
> that you are using the types from this header, why are you submitting this
> code to the binutils project ?  (Or maybe you are submitting it to both
> projects - I have not checked).
>
> In particular the BFD library has its own ELF reading and writing functions 
> and its own headers defining the layout of ELF structures.  Unfortunately 
> these headers do tend to conflict with the headers from the elfutils project, 
> whoch makes combining them problematical.

See my response to Joseph. This is basically because Solaris has
<gelf.h> easily available and used it promiscuously: we are only using a
few types that are always necessarily typedefs of types from <elf.h>,
but of course that's glibc-specific so I suppose we can't rely on that
either. But the binutils types seem... very far from ideal for my
purposes here, terribly bfd-specific.

Hence my suggestion (in an email that I hadn't written when you sent
this) that I could simply copy the necessary types from the installed
glibc headers into libctf. I don't know if that's too ugly to live.

>> +/* Simple doubly-linked list append routine.  This implementation assumes that
>> +   each list element contains an embedded ctf_list_t as the first member.
>> +   An additional ctf_list_t is used to store the head (l_next) and tail
>> +   (l_prev) pointers.  The current head and tail list elements have their
>> +   previous and next pointers set to NULL, respectively.  */
>
> You knows this kind of code seems awfully familiar.  I am sure that I have seen
> it implemented in lots of different places... :-)

Yes, but what else can we use? I tried using the stuff in <sys/queue.h>
and my eyes melted from all the CAPITAL LETTERS. :)

>> +void
>> +ctf_list_prepend (ctf_list_t * lp, void *new)
>
> I think that using "new" here might be a problem if you try to compile this
> source file with a C++ compiler.

True! I was only thinking in terms of using the headers with a C++
compiler... adjusted. However, there are a lot of other things we need
to fix up before libctf is C++-ready: implicit conversions to/from void
* are most of the problems, but we also use the %zi printf format in
several places...

>> +const char *
>> +ctf_strraw (ctf_file_t *fp, uint32_t name)
>> +{
>> +  ctf_strs_t *ctsp = &fp->ctf_str[CTF_NAME_STID (name)];
>
> My inner paranoia is screaming at code like this.  Unless you
> are certain that these functions cannot be called with out of 
> range parameters then I would strongly urge checking them before
> using them.

Possibly a nicer fix than explicitly checking is to change CTF_NAME_STID
a bit to mask, from:

#define CTF_NAME_STID(name)		((name) >> 31)

to

#define CTF_NAME_STID(name)		(((name) >> 31) & 1)

That should mask out all but the bottom bit, ensuring that even if
someone manages to pass a 64-bit value with 30 1-bits to CTF_NAME_STID
(... which cannot happen in ctf_strraw() as presently constituted, but a
later change might break that assmuption), the result of CTF_NAME_STID()
is always either 0 or 1.  Thus it will always fit into ctf_str and never
overwrite anything.

... or is this just piling too-cleverness on top of too-cleverness?

(I've done that, provisionally, for v2.)