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-0600 | Date Closed: | 2016-10-30 13:41:55 |
Priority: | Trivial | Regression? | |
Status: | Closed/Complete | Components: | 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 |