CODEWITHSUNDEEP
cstrcat
Darkmage
Darkmage

Reputation: 1603

Ways to build string in c

I'm building a string here based on 4 arguments and calling it with system(), but seems kinda messy the way i have done it. Is there a more correct way i should have done this and not use all those strcat and str1-4? 

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char *argv[])
{

    char str1[40] = "sed -n 's/.*\\(";
    char str2[] = "\\)\\(.*\\)\\(";
    char str3[] = "\\).*/\\2/p' ";
    char str4[] = " > ";

    if (argc != 5)
    {
        fprintf (stderr, "Usage %s <LogFile> <token1> <token2> <DumpFile>\n",
                argv[0]);
                exit(EXIT_FAILURE);
    }

    strcat(str1, argv[2]);
    strcat(str1, str2);
    strcat(str1, argv[3]);
    strcat(str1, str3);
    strcat(str1, argv[1]);
    strcat(str1, str4);
    strcat(str1, argv[4]);

    system(str1);

    return 0;
}

Upvotes: 3

Views: 7360

Answers (6)

cnicutar
cnicutar

Reputation: 182619

One problem with your code is that you're not checking the arguments fit in the 40 bytes.

I would probably use snprintf:

snprintf(str, LENGTH, "sed -n 's/.*\\(%s...", argv[2]...);

Upvotes: 8

David Heffernan
David Heffernan

Reputation: 612914

If you want your code to handle arbitrarily long arguments then you will need to allocate the character buffer dynamically with malloc.

  1. Create a local variable to calculate the total length required and use repeated calls to strlen to calculate that length.
  2. Call malloc remembering to add one more character for the null-terminator.
  3. Use strcpy or strcat multiple times to build up the string.
  4. Call system.
  5. Call free (or don't bother if your process is about to terminate).

Upvotes: 1

Jonathan Leffler
Jonathan Leffler

Reputation: 753665

This saves on the quadratic behaviour of repeated strcat(). OTOH, that is lost in the noise if you call system().

char str1[4096];
char str0[] = "sed -n 's/.*\\(";

sprintf(str1, "%s%s%s%s%s%s%s%s", str0, argv[2], str2, argv[3], str3, argv[1], str4, argv[4]);

You could use snprintf() if you're worried about buffer overflows (and you should be; 40 bytes in str1 is not enough; you left the 96 off the end, as in char str1[4096];). You could check the return value to see how many characters got written.

Upvotes: 3

jweyrich
jweyrich

Reputation: 32240

str1 is only 40 bytes long, and you're appending too much data to it. An stack overflow is likely to occur. I would do:

char buffer[1000]; // Choose a reasonable size
snprintf(buffer, sizeof(buffer),
    "sed -n 's/.*\\(%s\\)\\(.*\\)\\(%s\\).*/\\2/p' %s > %s",
    argv[2], argv[3], argv[1], argv[4]);

Upvotes: 2

asaelr
asaelr

Reputation: 5456

The only problem is that you may get buffer overflow (if the input is too long). to fix it, check the length of the strings (using strlen), and allocate enough memory to contain the string you want.

After you allocated enough memory, you can use a loop, or let sprintf do the work for you.

Upvotes: 2

rerun
rerun

Reputation: 25495

There is always sprintf which will make your life simpler. Make sure if you use sptrintf the buffer is large enough for the result. There is also a safer version snprintf which will do the bounds checking for you.

Upvotes: 2

Related Questions