http://www.slony.info/bugzilla/show_bug.cgi?id=67

           Summary: Array overrun in logtrigger()
           Product: Slony-I
           Version: devel
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: medium
         Component: trigger SPI
        AssignedTo: slony1-bugs at lists.slony.info
        ReportedBy: aburacze at gmail.com
                CC: slony1-bugs at lists.slony.info
   Estimated Hours: 0.0


Created an attachment (id=24)
 --> (http://www.slony.info/bugzilla/attachment.cgi?id=24)
A simple patch to detect the end of attkind string.

I submitted this bug to slony1-bugs, but found Bugzilla and thought it should
be placed here.

I think I found an array overrun bug in _Slony_I_logTrigger() function
(file: src/backend/slony1_funcs.c). The problematic array is "attkind"
trigger parameter, which is a zero-terminated string containing
letters 'k' and 'v'. These letters determine if the given table column
is a key or an ordinary value. In Slony-I 1.x this string had the
length equal to the number of the columns of the table. However,
starting from Slony-I 2.0.0, this string is trimmed and the trailing
'v' letters are removed. Here is the code from
determineAttkindUnique(text, name) function (file:
src/backend/slony1_funcs.sql, line 4807):

   -- Strip off trailing v characters as they are not needed by the logtrigger
   v_attkind := pg_catalog.rtrim(v_attkind, 'v');

As a result, the "attkind" parameter passed to the
_Slony_I_logTrigger() trigger function is usually shorter than the
number of colunms of the table, but this fact is not taken into
account in the function code. Here is an example (slony1_funcs.c, line
688, but the same is in the loop beginning in line 758):

       for (i = 0, attkind_idx = -1; i < tg->tg_relation->rd_att->natts; i++)
       {
               if (tupdesc->attrs[i]->attisdropped)
                       continue;
               attkind_idx++;
               if (attkind[attkind_idx] != 'k')
                       continue;
               /* The rest of the loop... */
       }

These loops iterate over all non-dropped columns of the table and
increment attkind_idx. Since the attkind can be shorter than the
number of columns, the attkind[attkind_idx] expression can read bytes
behind the end of the array. These bytes could have any random value,
including 0x6B (the ASCII code of 'k'), which could lead to random
treatment of an ordinary column as a key column (in fact, we observed
that in our test environment and that's why I investigated the issue).

I attached a small patch against slony1_funcs.c which checks for the 0
byte at the end of the attkind string and stops the iteration. This
patch also adds some error checking in the loop beginning in line 648.
Of course the problem can be removed by not trimming the string in the
determineAttkindUnique(text, name) function, but I think that the end
of a zero-terminated string should be checked in any case.


-- 
Configure bugmail: http://www.slony.info/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.