[SR-Users] valgrind errors from 3.1-stable (git 2fe4d633e4ac)

[Timo Teräs]

On 04/01/2011 11:17 AM, marius zbihlei wrote:
> On 04/01/2011 10:38 AM, Timo Teräs wrote:
>> On 04/01/2011 10:29 AM, marius zbihlei wrote:
>>> On 03/31/2011 07:52 PM, Timo Teräs wrote:
>> The first really evil programming mistake I found is at:
>> modules_k/textops/txtvar.c:tr_txt_eval_re()
>>
>> In line 86, it will fills the transformed variable with tr_txt_buf and
>> returns that. However, that is a stack variable, which will get
>> corrupted pretty soon after function return. I'm not sure how to fix
>> this properly.
>>
>>    
> Good call. I truly belive that should a "static" keyword in front of the
> buffer declaration. Can you test it. (because in the cfg you usually
> assign that transformation result to another pvar, the buffer will be
> deep copied soon enough).

static buffers are not nice solution and can break depending on the call
chain. However, since if it's going to be deep copied anyway, it'll
probably work here. I was not sure about the deep copying part, so I
hesitated to suggest it as possible fix.

> diff --git a/modules_k/textops/txt_var.c b/modules_k/textops/txt_var.c
> index ae1fce6..a13e5e1 100644
> --- a/modules_k/textops/txt_var.c
> +++ b/modules_k/textops/txt_var.c
> @@ -41,7 +41,7 @@ int tr_txt_eval_re(struct sip_msg *msg, tr_param_t
> *tp, int subtype,
>         int nmatches;
>         str* result;
>  #define TR_TXT_BUF_SIZE        2048
> -       char tr_txt_buf[TR_TXT_BUF_SIZE];
> +       static char tr_txt_buf[TR_TXT_BUF_SIZE];
> 
>         if(val==NULL || (!(val->flags&PV_VAL_STR)) || val->rs.len<=0)
>                 return -1;

Yes, I tried this actually earlier right after sending my previous mail.
It seems that this cured all the remaining valgrind errors I was seeing.

So yes, this should definitely pushed to all applicable git repos/branches.

>>>> ==22274== Invalid read of size 1
>>>> ==22274==    at 0x6DA2C0A: pv_set_ruri_user (pv_core.c:1755)
>>>> ==22274==    by 0x13BA15: lval_pvar_assign (lvalue.c:357)
>>>> ==22274==    by 0x13BF0F: lval_assign (lvalue.c:405)
>>>> ==22274==    by 0x139E4D: do_action (action.c:1472)
>>>> ==22274==    by 0x13A6F1: run_actions (action.c:1555)
>>>> ==22274==    by 0x12FDEB: do_action (action.c:711)
>>>> ==22274==    by 0x13A6F1: run_actions (action.c:1555)
>>>> ==22274==    by 0x13A86F: run_actions_safe (action.c:1607)
>>>> ==22274==    by 0x1E2666: rval_get_int (rvalue.c:904)
>>>> ==22274==    by 0x1E5252: rval_expr_eval_int (rvalue.c:1866)
>>>> ==22274==    by 0x131B8A: do_action (action.c:1071)
>>>> ==22274==    by 0x13A6F1: run_actions (action.c:1555)
>>>>
>>>> Is fairly obvious. modules_k/pv/pv_core.c has several places which take
>>>> backup of "val->rs.s[val->rs.len]" and replaces it with zero for string
>>>> termination. It's than later on set back to the original value.
>>>> However,
>>>> it would appear that the value passed does not always point to a memory
>>>> area which is large enough. This results in multiple invalid reads and
>>>> writes of one.
>>>>        
>> I fixed this by making a temporary copy of the string data to local
>> stack variable and null terminating it there. That made valgrind happy.
>> So obviously pv_set_ruri_user and others, are called with strings that
>> are allocated for the exact length, and the hacky zero-termination
>> cannot be used in this module.
>>    
> Well, indeed that's a ugly hack. As str's are not 0 terminated, you
> either copy to a buffer length+1 or you overwrite the memory location
> next to the end and revert the change afterwards. Not the cleanest way
> to do it, but from a performance point of view, it make sense. If should
> be taken extra care to ensure that on every code path the backup is
> restored.

Yes. My argument was that it absolutely does not work in two cases:
 - the memory allocated for the string is exactly the strings length
   (you'd be overwriting random places with the string terminator)
 - the same memory area is used for two overlapping strings: changing
   one byte would change all string references using that area

Though, in this instance the second item does not seem possible. I'm not
sure if the first item can happen or not.

>>>> The remaining reads are not so clear to me.
>>>>        
>> I'm analysing the rest problems. Seems that they are almost always
>> related to using re.subst, which has the problem mentioned in the
>> beginning of the mail.

Yes. Looks like all the nags were related to usage re.subst results.

Thanks,
  Timo