Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

2019-06-24 Thread Frederic Lecaille

Hi Ilya,

On 6/24/19 8:25 AM, Илья Шипицин wrote:

I had to be mroe detailed :)

I meant "simple reg test" + address sanitizer which we run in travis

address sanitizer is capable of catching those things



No, we do not write reg test anymore for bugs which have very few chance 
to come back.


Fred.



Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

2019-06-24 Thread William Lallemand


On Sun, Jun 23, 2019 at 10:10:12PM +0200, Tim Duesterhus wrote:
> Consider this configuration:
> 
> frontend fe_http
>   mode http
>   bind *:8080
> 
>   default_backend be_http
> 
> backend be_http
>   mode http
>   server example example.com:80
> 
> program foo bar
> 
> Running with valgrind results in:
> 
> ==16252== Invalid read of size 8
> ==16252==at 0x52AE3F: cfg_parse_program (mworker-prog.c:233)
> ==16252==by 0x4823B3: readcfgfile (cfgparse.c:2180)
> ==16252==by 0x47BCED: init (haproxy.c:1649)
> ==16252==by 0x404E22: main (haproxy.c:2714)
> ==16252==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
> 
> Check whether `ext_child` is valid before attempting to free it and its
> contents.
> 
> This bug was introduced in 9a1ee7ac31c56fd7d881adf2ef4659f336e50c9f.
> This fix must be backported to HAProxy 2.0.

Thanks, merged.

Also, I renamed the tag from "mworker" to "mworker-prog".

-- 
William Lallemand



Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

2019-06-24 Thread Илья Шипицин
I had to be mroe detailed :)

I meant "simple reg test" + address sanitizer which we run in travis

address sanitizer is capable of catching those things

пн, 24 июн. 2019 г. в 11:18, Илья Шипицин :

> Does it make sense to also add regtest on that regression?
>
> On Mon, Jun 24, 2019, 1:14 AM Tim Duesterhus  wrote:
>
>> Consider this configuration:
>>
>> frontend fe_http
>> mode http
>> bind *:8080
>>
>> default_backend be_http
>>
>> backend be_http
>> mode http
>> server example example.com:80
>>
>> program foo bar
>>
>> Running with valgrind results in:
>>
>> ==16252== Invalid read of size 8
>> ==16252==at 0x52AE3F: cfg_parse_program (mworker-prog.c:233)
>> ==16252==by 0x4823B3: readcfgfile (cfgparse.c:2180)
>> ==16252==by 0x47BCED: init (haproxy.c:1649)
>> ==16252==by 0x404E22: main (haproxy.c:2714)
>> ==16252==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
>>
>> Check whether `ext_child` is valid before attempting to free it and its
>> contents.
>>
>> This bug was introduced in 9a1ee7ac31c56fd7d881adf2ef4659f336e50c9f.
>> This fix must be backported to HAProxy 2.0.
>> ---
>>  src/mworker-prog.c | 30 --
>>  1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/mworker-prog.c b/src/mworker-prog.c
>> index 467ce9b24..ba52406e9 100644
>> --- a/src/mworker-prog.c
>> +++ b/src/mworker-prog.c
>> @@ -230,22 +230,24 @@ int cfg_parse_program(const char *file, int
>> linenum, char **args, int kwm)
>> return err_code;
>>
>>  error:
>> -   LIST_DEL(_child->list);
>> -   if (ext_child->command) {
>> -   int i;
>> -
>> -   for (i = 0; ext_child->command[i]; i++) {
>> -   if (ext_child->command[i]) {
>> -   free(ext_child->command[i]);
>> -   ext_child->command[i] = NULL;
>> +   if (ext_child) {
>> +   LIST_DEL(_child->list);
>> +   if (ext_child->command) {
>> +   int i;
>> +
>> +   for (i = 0; ext_child->command[i]; i++) {
>> +   if (ext_child->command[i]) {
>> +   free(ext_child->command[i]);
>> +   ext_child->command[i] = NULL;
>> +   }
>> }
>> +   free(ext_child->command);
>> +   ext_child->command = NULL;
>> +   }
>> +   if (ext_child->id) {
>> +   free(ext_child->id);
>> +   ext_child->id = NULL;
>> }
>> -   free(ext_child->command);
>> -   ext_child->command = NULL;
>> -   }
>> -   if (ext_child->id) {
>> -   free(ext_child->id);
>> -   ext_child->id = NULL;
>> }
>>
>> free(ext_child);
>> --
>> 2.21.0
>>
>>
>>


Re: [PATCH 4/9] BUG/MINOR: mworker: Fix segmentation fault during cfgparse

2019-06-24 Thread Илья Шипицин
Does it make sense to also add regtest on that regression?

On Mon, Jun 24, 2019, 1:14 AM Tim Duesterhus  wrote:

> Consider this configuration:
>
> frontend fe_http
> mode http
> bind *:8080
>
> default_backend be_http
>
> backend be_http
> mode http
> server example example.com:80
>
> program foo bar
>
> Running with valgrind results in:
>
> ==16252== Invalid read of size 8
> ==16252==at 0x52AE3F: cfg_parse_program (mworker-prog.c:233)
> ==16252==by 0x4823B3: readcfgfile (cfgparse.c:2180)
> ==16252==by 0x47BCED: init (haproxy.c:1649)
> ==16252==by 0x404E22: main (haproxy.c:2714)
> ==16252==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
>
> Check whether `ext_child` is valid before attempting to free it and its
> contents.
>
> This bug was introduced in 9a1ee7ac31c56fd7d881adf2ef4659f336e50c9f.
> This fix must be backported to HAProxy 2.0.
> ---
>  src/mworker-prog.c | 30 --
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/src/mworker-prog.c b/src/mworker-prog.c
> index 467ce9b24..ba52406e9 100644
> --- a/src/mworker-prog.c
> +++ b/src/mworker-prog.c
> @@ -230,22 +230,24 @@ int cfg_parse_program(const char *file, int linenum,
> char **args, int kwm)
> return err_code;
>
>  error:
> -   LIST_DEL(_child->list);
> -   if (ext_child->command) {
> -   int i;
> -
> -   for (i = 0; ext_child->command[i]; i++) {
> -   if (ext_child->command[i]) {
> -   free(ext_child->command[i]);
> -   ext_child->command[i] = NULL;
> +   if (ext_child) {
> +   LIST_DEL(_child->list);
> +   if (ext_child->command) {
> +   int i;
> +
> +   for (i = 0; ext_child->command[i]; i++) {
> +   if (ext_child->command[i]) {
> +   free(ext_child->command[i]);
> +   ext_child->command[i] = NULL;
> +   }
> }
> +   free(ext_child->command);
> +   ext_child->command = NULL;
> +   }
> +   if (ext_child->id) {
> +   free(ext_child->id);
> +   ext_child->id = NULL;
> }
> -   free(ext_child->command);
> -   ext_child->command = NULL;
> -   }
> -   if (ext_child->id) {
> -   free(ext_child->id);
> -   ext_child->id = NULL;
> }
>
> free(ext_child);
> --
> 2.21.0
>
>
>