[Home]

Summary:ASTERISK-25720: core: Make channel variable allocation easier to read
Reporter:Badalian Vyacheslav (slavon)Labels:
Date Opened:2016-01-23 01:34:04.000-0600Date Closed:2016-10-30 13:41:55
Priority:TrivialRegression?
Status:Closed/CompleteComponents:Core/General
Versions:13.7.0 Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chanvars.diff
Description:Founded by Asan
Comments:By: Asterisk Team (asteriskteam) 2016-01-23 01:34:05.337-0600

Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution.

A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report.

Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process].
By: Joshua C. Colp (jcolp) 2016-01-25 06:28:49.025-0600

Can you explain why you made this fix?

Originally 1 memory allocation was done with enough space to hold everything. This is a common practice used in the tree. Now you've made it into 3 different memory allocations. I don't see why there was a problem before.
By: Badalian Vyacheslav (slavon) 2016-01-25 06:44:34.929-0600

>  if (!(var = ast_calloc(sizeof(*var) + name_len + value_len, sizeof(char)))) {

# {{sizeof(char)}} != 1 byte for some platforms....
# reallocation of char[] or char[0] its bad way for compiller optimization and kernel memory overflow detect tools becouse char[0] its stack allocated var, but you reallocate it to global memory
# unknown behavor to compiler. try {{struct A {char B[0]; char C[0];} }} . Whats size of struct? Whats address for A.B and A.C ? Different compilers do different result/
By: Joshua C. Colp (jcolp) 2016-01-25 06:51:35.771-0600

Okay, so that should be updated to use the ast_calloc(1, ) way.

As for 2 that is used in GCC to specify a variable-length object. This is used in a few places in the tree.
By: Badalian Vyacheslav (slavon) 2016-01-25 06:53:47.416-0600

> Okay, so that should be updated to use the ast_calloc(1, ) way

Yep

> As for 2 that is used in GCC to specify a variable-length object. This is used in a few places in the tree.

Ok if you support only gcc compillers
By: Joshua C. Colp (jcolp) 2016-01-25 06:57:52.478-0600

Changing it otherwise has far reaching consequences, and would need to have the right path forward discussed.
By: Badalian Vyacheslav (slavon) 2016-01-25 07:13:01.428-0600

I'm not sure about {{char var[0]}} but
{code}
char var[1];
var = malloc(&var, ...);
{code}
it's bad error in gcc...
but
{code}
char var[];
var = malloc(&var, ...);
{code}
its good way becouse {{char var[] == char *var}}

By: Joshua C. Colp (jcolp) 2016-01-25 07:16:49.743-0600

That's standalone, not within a structure like is used for the channel variables.
By: Badalian Vyacheslav (slavon) 2016-01-25 07:22:15.959-0600

All the same, this is not standard behavior for C compilers and different reaction may be different. It is better to use standard methods to ensure correct behavior of different compilers. It is also easier to keep track of memory leaks.
By: Badalian Vyacheslav (slavon) 2016-01-25 07:29:43.172-0600

{{char name[0];}} - ok... this is may be corrent, but
{{char *value;}} - its POINER but you use it not for pointer!

i think my patch is clear to apply....

or you must do some one like this -

{code}
struct A{
char * name;
char * value;
char data[0];
}

A = calloc(1, sizeof(A) + strlen(some_name)+1 + strlen(some_val)+1);

memncpy(A.data, &some_name, strlen(some_name));
A.name = &A.data; // pointer to data buffer

memncpy(A.data + strlen(some_name) +1, &some_val,strlen(some_val));
A.value = A.data + strlen(some_name)+1; // pointer to data buffer
{code}
By: Joshua C. Colp (jcolp) 2016-01-25 07:34:16.039-0600

It is used for a pointer, it's updated to point to the memory at the end of the structure which was allocated to hold the value.
By: Badalian Vyacheslav (slavon) 2016-01-25 07:39:28.315-0600

No... you overflow address of {{name[0];}}!


By: Badalian Vyacheslav (slavon) 2016-01-25 07:43:55.210-0600

Sorry, i look better... no overflow ....
By: Badalian Vyacheslav (slavon) 2016-01-25 07:55:45.032-0600

i think my patch do:
# easy to read code
# easy to detect memory leaks
# more compillers support
# more accurate memory usage

and don't brake API
By: Joshua C. Colp (jcolp) 2016-01-25 08:05:32.874-0600

According to C99 sizeof(char) will always return 1:

6.5.3.4.3 When [sizeof operator is] applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1.

Have you actually seen platforms where this isn't true? If so then they would be violating C99.
By: Joshua C. Colp (jcolp) 2016-01-25 08:09:27.901-0600

You can put your patch up for review, otherwise it will be waiting for someone else to take it through the process.
By: Badalian Vyacheslav (slavon) 2016-01-25 08:10:23.299-0600

Some example...
> On theC2000 and C5000 DSP platforms, a char is 16 bits; on the C3x DSP generation, a char is 32 bits.

By: Joshua C. Colp (jcolp) 2016-01-25 08:21:33.021-0600

Despite that the C99 spec says that sizeof(char) *must* return 1.
By: Badalian Vyacheslav (slavon) 2016-02-08 05:34:33.828-0600

Sorry for long answer. Please post this patch to garret with minor prio.

I also rewrite this part becouse was memory leak, but i can't found false test results. Maybe was long night....

I believe that such code easier to read and therefore the likelihood of errors in this part of the code is reduced. Occam's Razor applies here as I think