[Asterisk-code-review] GCC12: Fixes for 16+ (asterisk[16])

[George Joseph]

Attention is currently required from: Kevin Harwell.
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18529 )

Change subject: GCC12: Fixes for 16+
......................................................................


Patch Set 1:

(3 comments)

File apps/app_festival.c:

https://gerrit.asterisk.org/c/asterisk/+/18529/comment/6861292d_6b2298fd 
PS1, Line 436: 	if (strlen(cachedir) + sizeof(MD5Hex) + 1 <= MAXFESTLEN && (usecache == -1)) {
> Not sure this is the right fix here? Two possible problems:
> 
> 1) sizeof(x) is not always equal to strlen(x)

In this case, MD5Hex is declared as char[33] so the sizeof allows the compiler to know the size in advance.  Apparently the compiler isn't smart enough to realize that the buffer is still fixed at 33 even using strlen.

> 2) with sizeof I think you can remove the "+ 1" since sizeof returns the full size of the "array" whereas strlen returns the length of the string -1 for NULL char.

We still have to account for the '/'.  The original should have used 2 to account for the NULL and the '/' so it was wrong to begin with.


File channels/chan_sip.c:

https://gerrit.asterisk.org/c/asterisk/+/18529/comment/baf54726_dd813d3c 
PS1, Line 35425: 		if ((void *)(expected_start) != (void *)start) {			\
> Curious what was the problem here? Comparison to NULL needs a cast now?

Actually, the change in line 35425 isn't technically needed but I played it safe.  The issue in line 35426 is that most callers of the CHECK_RESULTS pass in a value like "input + 8" and even though "input" is a "char *", if you add a const number to it, it can never be 0/NULL and will always evaluate to "true".  That's what the compiler was complaining about in the original ternary condition.  Just changing the ternary condition from (expected_start) to (expected_start != NULL) doesn't help because the compiler knows that NULL is "0" so it still complains.  You have to hide the fact that NULL is "0" from the compiler by casting it to "void *".  It's stupid I know but that's the way it is right now.


File include/asterisk/strings.h:

https://gerrit.asterisk.org/c/asterisk/+/18529/comment/5dd80de8_2e668150 
PS1, Line 396: 	volatile size_t sz = size;
             : 	volatile char *sp = (char *)src;
> I _think_ this breaks ABI as this is an inline function defined in a header, so altering stuff here could change the size of the resulting library.

If a module has inlined it, the code is in the module so changing it here has no effect on the module until the module recompiles.  At that point it'll pick up the new version.  Yeah?



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18529
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia081ca1bcfb329df6487c4660aaf1944309eb570
Gerrit-Change-Number: 18529
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 05 May 2022 12:15:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220505/7de24c57/attachment.html>