[asterisk-dev] (unreported) uninitialized: struct ast_sockaddr

[Richard Mudgett]

On Wed, May 13, 2015 at 3:59 AM, Alexander Traud <pabstraud at compuserve.com>
wrote:

> > What you're proposing is a change to the semantics of ast_sockaddr.
>
> Not sure what you mean by semantics. Please, let us ignore ast_sockaddr for
> a second and see <http://www.ex-parrot.com/~chris/random/initialise.html>:
>

Semantics just mean how something has to be used.  You are changing the
semantics of ast_sockaddr by adding something that must be explicitly
released.
Whereas, before there was nothing that needed to be explicitly released so
nothing would need to be explicitly called to destroy the ast_sockaddr
instance.


>
> Currently, when a struct with "automatic storage duration" is created in
> Asterisk, it is initialized
> A) (correct) with {0},
> B) (questionable) via memset,
> C) (questionable) at first use, or
> D) (wrong) not at all.
>
> Is case D important enough to get fixed, at a whole, or partially. If
> partially, to which extend?
>
> Example 1:
> The *opaque* peercnt (channels/chan_iax2.c) contains ast_sockaddr and
> therefore has to be initialized correctly in my case to avoid a wild
> pointer. Actually, I am using chan_sip only, added a pointer to
> ast_sockaddr, cleaned memory, and my Asterisk was segfaulting in a complete
> different module (chan_iax2.c). [Offtopic: Yes, my modules.conf was wrong.]
>

struct peercnt is not opaque as the struct definition is visible to the code
using the structure.  If it were opaque you would only be able to create
pointer
variables to the struct and not instances of the struct itself.  The
compiler
cannot let you create an instance of struct peercnt if it does not know the
size
of it.

struct peercnt *pointer_to_peercnt;
struct peercnt instance_of_peercnt;


>
> Example 2:
> The *private* sip_peer (channels/sip/include/sip.h) contains pointers and
> is
> not initialized at all twice, at least (sip_peer tmp_peer). This creates
> wild pointers which segfaulted the pointer in my ast_sockaddr.
>

These particular uses of peercnt in chan_iax2.c and sip_peer in chan_sip.c
are
dummy objects with just enough fields filled in to perform an ao2_find().
Those
dummy objects are never destroyed so the uninitialized pointers are
irrelevant.
The coding pattern that creates a dummy object with the container key fields
filled in pre-dates the introduction of OBJ_SEARCH_KEY and the older name
OBJ_KEY.


>
> Asked differently:
> I have a diff/patch correcting just the struct-inits for 62 lines of code
> at
> 54 places in 5 files, which affects my downstream code. Am I allowed to
> commit just that, although it does not address the issue at a whole (there
> are many more struct inits which stay wrong)?
>
> Or: Is my compiler configured incorrectly somehow, not setting pointers to
> (void *)0 automatically in structs with automatic storage duration?
>

I haven't seen a compiler that tracks uninitialized struct members.  You
should
make your addition as a char array rather than an allocated pointer to avoid
incompatibilities in unexpected places.  Otherwise, the semantic change
breaks code all over the place.

Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150513/ab24e8ad/attachment.html>