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

[Daniel-Constantin Mierla]

On 2/2/10 7:43 AM, Alex Balashov wrote:
> 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'.
>
I will look into int. Is the db column string or integer?

Cheers,
Daniel

-- 
Daniel-Constantin Mierla
eLearning class for Kamailio 3.0.0
Starting Feb 8, 2010
* http://www.asipto.com/