[Kamailio-Users] PV bug in modules_k/acc/acc_extra.c in K 3.0.0

[Alex Balashov]

I ran into a very interesting problem last night with 'acc' that took 
me a few hours to solve, so I am posting my workaround for collective 
benefit.  It is probably not the cleanest and most appropriate 
solution because I am not extensively familiar with the 'acc' code; 
this is my first dive into it.  Thus, I do not presume to submit an 
actual patch.

The problem I had was that PVs read from the db_extra parameter that 
had an internal type of integer (TYPE_INT) because of their lexical 
scope were not being correctly evaluated, resulting in bizarre values 
being put in the database.

I am using K 3.0.0 on 64-bit Xen guest running CentOS with 
db_postgres.  I did not check to see if this problem occurs with 
db_mysql as I do not have MySQL anywhere nearby, but I don't believe 
it is happening at the database layer anyhow.  I also know this 
problem was not present in 1.5.x, or several of my installations would 
be non-functional;  this must have something to do with use of new 
'srdb1' stuff in the database side of 'acc', I hypothesise.

Anyway, the actual problem went like this.  Let us say, for simplicity 
of example, that I am using 'acc' with Postgres backing and have 
something like this:

    modparam("acc", "db_extra", "account_id=$avp(s:account_id)")
    ...
    modparam("acc", "db_flag", 1)

What was happening was that if I did this in a request route:

    $avp(s:account_id) = 42;
    ...
    setflag(1);

    if(!t_relay())
       sl_reply_error();

    exit;

Very strange and occasionally bizarre values, mostly duplicates of 
other values I have in db_extra, would be written to acc.account_id (a 
column of type 'integer') - at any rate, certainly not 42.

I enabled SQL statement logging on my PostgreSQL database to ensure 
that the data being inserted was literally not 42, rather than this 
being some sort of database-layer type conversion / representation 
problem.  Sure enough, it was true, although I also did notice that 
'acc' encloses all inserted values in single quotes to be treated as 
string literals.  But this is immaterial, because RDBMs automatically 
cast such input:

    INSERT INTO acc (..., account_id) VALUES(..., '12');

Now, I could fix the problem by simply assigning the AVP a string 
literal instead:

    $avp(s:account_id) = '42';

Now the data was inserted normally.

I also tested with user variables ($var(...)) and the same was true, 
so the problem is not AVP-specific.  Also, all other evaluative 
contexts for PVs with these values work fine, such as printing them. 
The problem does not seem to be with 'db_postgres', neither with 'pv', 
nor with 'avpops' -- it is particular to 'acc'.

After some hours of investigation I found that what was happening is 
that the type of the PV being referenced is not being correctly packed 
into the various arrays populated by extra2strar() in 
modules_k/acc/acc_extra.c, and thus was being misread when iterated in 
acc_db_request() in modules_k/acc/acc.c.  Specifically, this check was 
inadequate:

                        if (value.rs.s+value.rs.len==static_detector) {
                                 val_arr[n].s = int_buf + 
r*INT2STR_MAX_LEN;
                                 val_arr[n].len = value.rs.len;
                                 memcpy(val_arr[n].s, value.rs.s, 
value.rs.len);
                                 r++;

                                 LM_INFO("New int tenant: %s\n", 
val_arr[n].s);
                         } else {
                                 val_arr[n] = value.rs;
                                 LM_INFO("New str tenant: %s\n", 
value.rs);
                         }


In my debugging, all PV values were showing up as "New str tenant" 
regardless of whether they were natively integers, and regardless of 
what the PV spec type was actually set to.  The type was set correctly 
on the PV spec, so of course the integer value got stored correctly in 
int_var[] below:

                         if (value.flags&PV_VAL_INT) {
                             int_arr[n] = value.ri;
                             type_arr[n] = TYPE_INT;
                         } else {
                             type_arr[n] = TYPE_STR;
                         }

... but since 'acc' wants to treat all extra values as strings, this 
was not sufficient, because what was failing was this condition:

                         if (value.rs.s+value.rs.len==static_detector)

I guess value.rs.s in case of the PV value being PV_VAL_INT is not 
null terminated properly?  This I truly do not know.

In any event, I just added a check on the PV flags to see if it is an 
integer value and this fixed my problem:

                         if (value.rs.s+value.rs.len==static_detector
                                 || value.flags & PV_VAL_INT) {

... however, I do not know that this was the most insightful solution.

This is rather critical I think, because it means anyone trying to 
store PV values that are initialised with a natively numerical type in 
script (or extracted as a numerical value from a database, e.g. 
sql_query("SELECT account_id::integer FROM table ..."), I found this 
to be a problem as well) cannot store them in the database reliably 
using 'db_extra'.

Thanks!

-- 
Alex Balashov - Principal
Evariste Systems LLC

Tel    : +1 678-954-0670
Direct : +1 678-954-0671
Web    : http://www.evaristesys.com/