A potential bug to squeeze extra memory through command line arguments

[haris iqbal]

On Fri, Feb 19, 2016 at 6:55 PM, haris iqbal <haris.phnx at gmail.com> wrote:
> On Fri, Feb 19, 2016 at 6:00 PM, Dmitry V. Levin <ldv at altlinux.org> wrote:
>> Hi,
>>
>> On Fri, Feb 19, 2016 at 05:47:40PM +0530, haris iqbal wrote:
>>> Hello.
>>>
>>> I was going through the code of strace.c, when I found this line in
>>> init() function.
>>>
>>> tcbtabsize = argc;  /* Surely enough for all -p args.  */
>>
>>> That set me thinking is it really is what I think it is. Can it really
>>> be that strace will allocate that many tcbtab pointers and tcp
>>> structures as many arguments I give to the program.
>>
>> The fun of the situation is that after commit v4.6-258-ge8172b7
>> this is no longer enough for all -p args.
>>
>>> So I set out to find out whether there is a way to make strace take a
>>> lot of memory, without actually using it.
>>>
>>> Firstly I found out that if there is a way to give large number of
>>> arguments and still make strace work as it should. It turns out that
>>> there is. One can give as many -i (or some other such arguments) as
>>> arguments and the strace would work properly.
>>>
>>> Then, I wanted to make sure whether the code is actually allocating
>>> that many resources as as there are arguments. So I went into the
>>> cleanup() function that is called through error_msg_and_die(). And
>>> there I added debugging code to see how many of them were actually
>>> allocated. It turns out that they were allocated.
>>> I added a print statement in the for loop of cleanup(). It ran for as
>>> many number of times as there were arguments. Although the usefull one
>>> was the first one only, which had the pid of the process being traced
>>> (strace was run with -p <pid> option).
>>>
>>> So, summing it all up. I ran a script which gave strace a -p option
>>> and a process pid to trace. Along with that some 600000 -i options. It
>>> ran successfully with a memory consumption of around 100mb. Thats a
>>> lot for a small system with limited memory.
>>
>> OK, so what would you suggest to change in initial tcb table allocation?
>
> There are still many portions of code which I have not understood
> properly, but I am to make a suggestion with whatever knowledge I have
> of the code. I would say to postpone the allocation till alloctcb() is
> called. But I can see many places where alloctcb() is called is by
> checking whether tcp->pid is present or not. And for that check, one
> would need an allocated structure with a 0 value.
>
> Are we allocating tcb table for multiple -p options only or does it
> have some other purpose?
>

Ok. I have come up with a separate memory model for tcbtab. In this
model, we will use a linked list instead of a global array of pointers
tcbtab.

The structure

struct s_tcbtab
{
    struct tcb* data;
    struct s_tcbtab* next;
}

And a global head of the linked list.

struct s_tcbtab* head_tcbtab = NULL;

Now, no need for the following code in init()

    /* Allocate the initial tcbtab.  */
    tcbtabsize = argc;  /* Surely enough for all -p args.  */


    tcbtab = xcalloc(tcbtabsize, sizeof(tcbtab[0]));
    tcp = xcalloc(tcbtabsize, sizeof(*tcp));
    for (tcbi = 0; tcbi < tcbtabsize; tcbi++)
        tcbtab[tcbi] = tcp++;

tcbtabsize will be 0.

The changes will be made in the alloctab(). The code that will be added is


if(head_tcbtab == NULL)
{
    // for the first allocation
    head_tcbtab = malloc(sizeof(s_tcbtab));
    head_tcbtab->next = NULL
    tcp = head_tcbtab -> data = malloc(sizeof(struct tcb));
    tcbtabsize++;
}
else
{
    // further adding tabs
    temp_tcb = head_tcbtab;
    while(temp_tcb->next != NULL)
        temp_tcb = temp_tcb->next;

    temp_tcb->next = malloc(sizeof(s_tcbtab));
    temp_tcb = temp_tcb->next;

    temp_tcb->next = NULL;
    tcp = temp_tcb->data = malloc(sizeof(struct tcb));
    tcbtabsize++;
}

after this, the code can continue with the normal

    memset(tcp, 0, sizeof(*tcp));
    tcp->pid = pid;
#if SUPPORTED_PERSONALITIES > 1
    tcp->currpers = current_personality;
    .
    .
    return tcp;

Additional code has to be changed at places where there is a for loop
iteration like in startup_attach()

we have to change this

    for (tcbi = 0; tcbi < tcbtabsize; tcbi++)
    {
        tcp = tcbtab[tcbi];
        ..

to

    tcbi = head_tcbtab;
    while(tcbi != NULL)
    {
        tcp = tcbi->data;
        ..

>>
>>
>> --
>> ldv
>>
>> ------------------------------------------------------------------------------
>> Site24x7 APM Insight: Get Deep Visibility into Application Performance
>> APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
>> Monitor end-to-end web transactions and take corrective actions now
>> Troubleshoot faster and improve end-user experience. Signup Now!
>> http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
>> _______________________________________________
>> Strace-devel mailing list
>> Strace-devel at lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/strace-devel
>>
>
>
>
> --
>
> With regards,
>
> Md Haris Iqbal,
> Placement Coordinator, MTech IT
> NITK Surathkal,
> Contact: +91 8861996962



-- 

With regards,

Md Haris Iqbal,
Placement Coordinator, MTech IT
NITK Surathkal,
Contact: +91 8861996962