Re: [asterisk-dev] (unreported) uninitialized: struct ast_sockaddr
On Wed, May 13, 2015 at 3:59 AM, Alexander Traud pabstr...@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
--
_
-- 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
Re: [asterisk-dev] (unreported) uninitialized: struct ast_sockaddr
On Mon, May 11, 2015 at 9:16 AM, Alexander Traud pabstr...@compuserve.com wrote: B) Change my patch not to use a char* but char[128]. Your easiest option with less chance of regression elsewhere would be this. Yes. Anyway: Is the Asterisk team interested in a patch at least for the 5 affected files? These are 54 changes. I am not sure if the path via an issue (Jira) and a commit (Gerrit) is welcome for something like this. Furthermore, again there are several options: I) in those 5 files, report only those 54 changes which I need; or II) in those 5 files, correct *all* struct inits The latter is interesting, because there are memsets(.) which get unnecessary then. This created an additional option/question: III) shall I remove all - then duplicate - struct inits as well? Or shall I stop wasting anyone's time and go for option B, a char[128]? I wouldn't say you're wasting anyone's time. It's a good question. What you're proposing is a change to the semantics of ast_sockaddr in order to accommodate a change in a patch you're making. A few things to keep in mind: 1) Your module/patch, as well as the rest of the Asterisk modules, may not be the only consumers of ast_sockaddr. It is a fundamental data structure in Asterisk; other third party modules may use it. Since your change modifies the semantics of the struct, you will also break them. 2) If the rest of your patch is never pushed back up to the main project, then the change you've made has little positive affect, while breaking most other external modules that use it. That's not good for the project. On the other hand, if you propose your feature for inclusion as well, there's more of a benefit to the change. That aside, if you can make a change with a little bit more work that maintains the existing usage semantics, you get the best of both worlds: your new feature gets what it needs, and the mainline Asterisk project - as well as other third party modules - aren't broken. So yes, I'd just use a char array with a fixed length. -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- 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
Re: [asterisk-dev] (unreported) uninitialized: struct ast_sockaddr
Alexander Traud wrote:
In a patch of mine (DANE for Asterisk 13/chan_sip; available on request), a
char* was added in the struct ast_sockaddr to store the DNSSEC failure
reason (why_bogus). Not to create any memory leaks, this pointer has to be
freed. For this, the pointer must be initialized to NULL, for example via
struct ast_sockaddr variable_name = { 0 }. Otherwise, I create a
segmentation fault because I free a non-valid address. Long story short:
Neither GCC nor Clang are able to find uninitialized structs in Asterisk,
although the warning flag -Wall includes -Wuninitialized. And I played
around with the optimization in CFLAG. Mhm.
What shall I do?
A) Init all ast_sockaddr (and all structures containing ast_sockaddr).
With the default sample configuration, just 5 files must be changed.
However, I searched with the regular expression struct [^ ]+ [^*=)]+; and
found 2408 lines of code in 418 files which may be uninitialized. As an
external project member, I cannot change/commit all of them. For a start,
shall I create issue report just about these 5 files?
B) Change my patch not to use a char* but char[128].
Your easiest option with less chance of regression elsewhere would be this.
--
Joshua Colp
Digium, Inc. | Senior Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - US
Check us out at: www.digium.com www.asterisk.org
--
_
-- 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
Re: [asterisk-dev] (unreported) uninitialized: struct ast_sockaddr
B) Change my patch not to use a char* but char[128]. Your easiest option with less chance of regression elsewhere would be this. Yes. Anyway: Is the Asterisk team interested in a patch at least for the 5 affected files? These are 54 changes. I am not sure if the path via an issue (Jira) and a commit (Gerrit) is welcome for something like this. Furthermore, again there are several options: I) in those 5 files, report only those 54 changes which I need; or II) in those 5 files, correct *all* struct inits The latter is interesting, because there are memsets(.) which get unnecessary then. This created an additional option/question: III) shall I remove all - then duplicate - struct inits as well? Or shall I stop wasting anyone's time and go for option B, a char[128]? -- _ -- 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
