[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] (unreported) uninitialized: struct ast_sockaddr
From: Richard Mudgett <rmudgett () digium ! com>
Date: 2015-05-13 16:57:59
Message-ID: CALD46g1mCptB_4-cJ+3CO6c3VrGC26KaO4V5OUvepGwmFYZ_KA () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Wed, May 13, 2015 at 3:59 AM, Alexander Traud <pabstraud@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
[Attachment #5 (text/html)]
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May \
13, 2015 at 3:59 AM, Alexander Traud <span dir="ltr"><<a \
href="mailto:pabstraud@compuserve.com" \
target="_blank">pabstraud@compuserve.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">> What you're proposing is a change to the \
semantics of ast_sockaddr.<br> <br>
Not sure what you mean by semantics. Please, let us ignore ast_sockaddr for<br>
a second and see <<a href="http://www.ex-parrot.com/~chris/random/initialise.html" \
target="_blank">http://www.ex-parrot.com/~chris/random/initialise.html</a>>:<br></blockquote><div><br></div><div>Semantics \
just mean how something has to be used. You are changing the<br>semantics of \
ast_sockaddr by adding something that must be explicitly released.<br>Whereas, before \
there was nothing that needed to be explicitly released so<br>nothing would need to \
be explicitly called to destroy the ast_sockaddr instance.<br></div><div> \
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px \
solid rgb(204,204,204);padding-left:1ex"> <br>
Currently, when a struct with "automatic storage duration" is created \
in<br> Asterisk, it is initialized<br>
A) (correct) with {0},<br>
B) (questionable) via memset,<br>
C) (questionable) at first use, or<br>
D) (wrong) not at all.<br>
<br>
Is case D important enough to get fixed, at a whole, or partially. If<br>
partially, to which extend?<br>
<br>
Example 1:<br>
The *opaque* peercnt (channels/chan_iax2.c) contains ast_sockaddr and<br>
therefore has to be initialized correctly in my case to avoid a wild<br>
pointer. Actually, I am using chan_sip only, added a pointer to<br>
ast_sockaddr, cleaned memory, and my Asterisk was segfaulting in a complete<br>
different module (chan_iax2.c). [Offtopic: Yes, my modules.conf was \
wrong.]<br></blockquote><div><br></div><div>struct peercnt is not opaque as the \
struct definition is visible to the code<br></div><div>using the structure. If it \
were opaque you would only be able to create pointer<br>variables to the struct and \
not instances of the struct itself. The compiler<br>cannot let you create an \
instance of struct peercnt if it does not know the size<br>of \
it.<br><br></div><div>struct peercnt *pointer_to_peercnt;<br></div><div>struct \
peercnt instance_of_peercnt;<br></div><div> <br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"> <br>
Example 2:<br>
The *private* sip_peer (channels/sip/include/sip.h) contains pointers and is<br>
not initialized at all twice, at least (sip_peer tmp_peer). This creates<br>
wild pointers which segfaulted the pointer in my \
ast_sockaddr.<br></blockquote><div><br></div><div>These particular uses of peercnt in \
chan_iax2.c and sip_peer in chan_sip.c are<br>dummy objects with just enough fields \
filled in to perform an ao2_find(). Those<br>dummy objects are never destroyed so \
the uninitialized pointers are irrelevant.<br>The coding pattern that creates a dummy \
object with the container key fields<br>filled in pre-dates the introduction of \
OBJ_SEARCH_KEY and the older name<br>OBJ_KEY.<br></div><div> </div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"> <br>
Asked differently:<br>
I have a diff/patch correcting just the struct-inits for 62 lines of code at<br>
54 places in 5 files, which affects my downstream code. Am I allowed to<br>
commit just that, although it does not address the issue at a whole (there<br>
are many more struct inits which stay wrong)?<br>
<br>
Or: Is my compiler configured incorrectly somehow, not setting pointers to<br>
(void *)0 automatically in structs with automatic storage \
duration?<br></blockquote><div><br></div><div>I haven't seen a compiler that \
tracks uninitialized struct members. You should<br></div><div>make your addition as \
a char array rather than an allocated pointer to avoid<br>incompatibilities in \
unexpected places. Otherwise, the semantic change<br>breaks code all over the \
place.<br><br></div><div>Richard<br></div></div><br></div></div>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic