String concat in C

Question

void xsdValidation(char *xsdName, char *xmlName){
 char *terminalCommand;
 system("xmllint --noout --schema ", xsdName , xmlName);
}

I have a little problem with that. I have a code to valide my xml. And my xml's name and xsd's name came from as a argument. How can i concat these 3 things?

You want to have a look at `strcpy()` and/or `strcat()`, for example. — alk, Mar 06 '16 at 13:33

fuz · Answer 1 · 2016-03-06T14:03:12.817

3

Use snprintf():

/* determine buffer size */
int len = snprintf(NULL, 0, "xmllint, --noout --schema %s %s", xsdName, xmlName);
if (len < 0) {
    /* error handling for EILSEQ here */
}

char *buf = malloc(len + 1);
if (buf == NULL) {
    /* err handling for malloc() failure here */
}

snprintf(buf, (size_t)len, "xmllint, --noout --schema %s %s", xsdName, xmlName);

system(buf);
free(buf);

On a sufficiently recent system, you could also use asprintf() to greatly simplify this code:

char *buf = NULL;
asprintf(&buf, "xmllint, --noout --schema %s %s", xsdName, xmlName);
if (buf == NULL) {
    /* error handling here */
}

system(buf);
free(buf);

Note that all these approaches fail if xsdName or xmlName contain spaces or other special characters. You might want to invoke an exec function directly to avoid that problem.

edited Mar 06 '16 at 14:03

answered Mar 06 '16 at 13:35

fuz

88,405
25
200
352

You need to add one to `len` in the `malloc` – Ed Heal Mar 06 '16 at 13:36
My only suggestions would be: 1. Use VLAs rather than `malloc`, as that way you wouldn't need to use `free` 2. Remember to use `free` if you still choose to use `malloc`... – autistic Mar 06 '16 at 13:43
@EdHeal Variable length array... `char buf[len + 1];` rather than `char *buf = malloc(len + 1);`... Another benefit is, no `NULL` checking is required for `buf` when it's a VLA. Overall, the code is simplified quite drastically. – autistic Mar 06 '16 at 13:50
@Seb VLAs are inacceptable as they don't have a good failure mode in case memory allocation fails. What if the user inputs a very long file name? – fuz Mar 06 '16 at 14:02
@FUZxxl I think that's something you'd have to ask the OP. Nonetheless, the same problem exists for `malloc`, in fact VLAs may be implemented behind the scenes using `malloc`. What happens if a user enters a filename that's larger than `SIZE_MAX` bytes? – autistic Mar 06 '16 at 14:05
@FUZxxl Have you considered that `main` might also be unacceptable as it doesn't have a good failure mode, by that logic? There's no guarantee within the C standard that there'll be enough resources for the program to start or allocate locals, right? I have now raised two problems with any alternative to VLAs that you can come up with. There are no solutions to these problems in C. – autistic Mar 06 '16 at 14:11
@Seb `malloc()` failure can be caught as `malloc()` returns `NULL` in this case. `xsdName` cannot be larger than `SIZE_MAX` by definition of `size_t`. Stack space is usually fairly limited, allocating user data of arbitrary size on the stack is a bad idea and a recipe for security problems and crashes. A stack overflow is something you cannot gracefully handle without a lot of effort. This does not apply to `malloc()` as `malloc()` tells you when it fails. The rest of your argument is trolling and I shall not respond to it. – fuz Mar 06 '16 at 14:46
@FUZxxl ... and where do you store the return value? What if that allocation fails, hmm? Your solution does not solve the problem you claim that it solves. Do you think, of all of the vulnerabilities possible in the idea this question is asking about, that a stack overflow takes the cake? Why is it that you're concerned about this and not the others? Personally, I don't think this is a security-critical application, and by making it one you're the pedant here. – autistic Mar 06 '16 at 18:16
@Seb When `xsdName` and `xmlName` come from user input, they can become large. Stack space is usually farily limited. This is not a problem with “normal” stack usage as a single stack frame is only a couple hundreds of bytes large usually. However, when you allow the user to cause arbirarily large allocations on the stack (e.g. by using variable-length arrays with unrestricted length) it is very easy for a malicious user to exhaust the available stack space. This generally causes a program crash or worse in case the stack is overrun by so much that the stack pointer points into unrelated data. – fuz Mar 06 '16 at 18:32
The same thing cannot happen with memory obtained via `malloc()`: If not enough space is available, `malloc()` fails. If the stack is used normally, there is enough space to store the return value of `malloc()`. Likely, that return value never ends up on the stack anyway. You are purposely being ignorant here. The chance of overflowing the stack with normal usage (i.e. only very small objects, shallow recursion if at all, etc.) is extremely low. In a nonrecursive program, it can be proven to be impossible. If you give the user a chance to allocate an arbitrary file name on the stack, ... – fuz Mar 06 '16 at 18:35
... overflowing it becomes extremely easy. Please point out the other security issues the code has. I'm not aware of any. And stop trolling, please. – fuz Mar 06 '16 at 18:36
@FUZxxl Do you think the OP is going to bother entering 4MB filenames in order to compromise a machine he already owns? That you have to explain this to me should be insulting to me, but it's not... It's insulting to you as you didn't deduce from my previous comments that I already had a fairly good understanding of what you're talking about. Apparently you don't, however. **If we're talking about vulnerabilities here, consider the effect that a pipe will have on the `system()` call... Why are you only focusing on one vulnerability, hmmm?** – autistic Mar 06 '16 at 19:02
@Seb So you don't think a programmer should be security-concious? And I already warned about the risk of special characters in the file names, yes I am concerned about that. – fuz Mar 06 '16 at 19:26
@FUZxxl I don't think it's absolutely necessary to be security conscious in every piece of software, no. If the user of the program already administrates the system, there's nothing he/she could stand to gain by exploiting a vulnerability. I seriously doubt he/she is going to sit there hitting whatever combination of keys is required to insert enough null opcodes to build up a nopsled, followed by tediously reproducing a payload and then overflowing the stack with four million bytes just to gain access to a system he/she already has full access to. – autistic Mar 06 '16 at 19:38
@Seb You know, instead of discussing for one hours whether or not it is acceptable not to care about security, you could have just went ahead and implemented the `malloc`-based variant which isn't any more difficult to program. – fuz Mar 06 '16 at 19:40
@Seb Or, for even more performance (as you seem to be so much concerned about performance), you could call `execvp` directly without concatenating strings, without having problems with pipe characters in file names, etc, just as I already said in my answer. – fuz Mar 06 '16 at 19:41
@FUZxxl What fun would that be? – autistic Mar 06 '16 at 19:43

score 2 · Accepted Answer · answered Mar 06 '16 at 13:35

2

You can use strcat() to concat strings.

void xsdValidation(char *xsdName, char *xmlName){
 static const char *xmlLint = "xmllint --noout --schema ";
 /* do not forget to +1 for terminating null character */
 char *terminalCommand = malloc(strlen(xmlLint) + strlen(xsdName) + strlen(xmlName) + 1);
 if(terminalCommand != NULL){
  strcpy(terminalCommand, xmlLint); /* use strcpy() here to initialize the result buffer */
  strcat(terminalCommand, xsdName);
  strcat(terminalCommand, xmlName);
  system(terminalCommand);
  free(terminalCommand);
 }
}

This may improve performance a little because it reuse the calculated length.

void xsdValidation(char *xsdName, char *xmlName){
 static const char *xmlLint = "xmllint --noout --schema ";
 size_t xmlLintLen = strlen(xmlLint);
 size_t xsdNameLen = strlen(xsdName);
 size_t xmlNameLen = strlen(xmlName);
 /* do not forget to +1 for terminating null character */
 char *terminalCommand = malloc(xmlLintLen + xsdNameLen + xmlNameLen + 1);
 if(terminalCommand != NULL){
  /* use strcpy() to copy the strings */
  strcpy(terminalCommand, xmlLint);
  strcpy(terminalCommand + xmlLintLen, xsdName);
  strcpy(terminalCommand + xmlLintLen + xsdNameLen, xmlName);
  system(terminalCommand);
  free(terminalCommand);
 }
}

answered Mar 06 '16 at 13:35

MikeCAT

73,922
11
45
70

In the 2nd snippet: Interesting micro optimisation. – alk Mar 06 '16 at 13:38
saves an impressive number of picoseconds, if you run it billions of times. – Michel Billaud Mar 06 '16 at 13:41
If you want to save these precious nanoseconds, why not use `memcpy` in the second implementation? – fuz Mar 06 '16 at 13:42
Guys, `strlen()` indeed is a hell lot expensive. – alk Mar 06 '16 at 13:43
2

Compare it with the cost of the call to system() ! – Michel Billaud Mar 06 '16 at 13:44
Well, yes, err ... ok, fair enough! ;-) @MichelBillaud – alk Mar 06 '16 at 13:45
1

One important part of the equation is the waste of human programmer time – Michel Billaud Mar 06 '16 at 13:46
@alk Do you have proof to suggest that this needs optimisation? It seems to me, if the program is perceived to be fast enough by the user, it doesn't need optimisation, right? Please avoid (dramatic) premature optimisations from this point on... – autistic Mar 06 '16 at 13:47
For the common pattern of allocing followed by copying/concatting this *is* a nice/sane micro optimisation. Whether necessary or not indeed depends on the context. @Seb – alk Mar 06 '16 at 13:49
@Seb: And my comment did *not* mean to imply optimisation is needed in this question's context. – alk Mar 06 '16 at 13:51
@alk What alternative would you suggest to `strlen`? Without an alternative, technically there is no optimisation at all, and you can't talk about the sanity of an optimisation that doesn't exist. – autistic Mar 06 '16 at 13:53
@Seb: The optimisation is to not use it if not necessary. In the answer's 2nd snippet it's use use had been avoided twice by replacing `strcat()` by `strcpy()`. – alk Mar 06 '16 at 13:55
@alk That waste is not `strlen` but `strcat`... `strlen` is still clearly used in the second snippet. Nonetheless, there's another bottleneck that's likely to be much more significant than your problem with `strcat` (or `strlen`, or whatever you decide it is)... and that is `malloc`. Less premature optimisation would be a good idea. – autistic Mar 06 '16 at 14:02

score 0 · Answer 3 · answered Mar 06 '16 at 13:42

If the compiler supports variable length arrays then you may write

void xsdValidation( const char *xsdName, const char *xmlName )
{
    const char *command = "xmllint --noout --schema ";
    char terminalCommand[strlen( command ) + strlen( xsdName ) + strlen( xmlName ) + 2];

    strcpy( terminalCommand, command );
    strcat( terminalCommand, xsdName );
    strcat( terminalCommand, " " );
    strcat( terminalCommand, xmlName );

    system( terminalCommand );
}

Otherwise you should to allocate the required array dynamically. For example

void xsdValidation( const char *xsdName, const char *xmlName )
{
    const char *command = "xmllint --noout --schema ";
    char *terminalCommand = 
        malloc( strlen( command ) + strlen( xsdName ) + strlen( xmlName ) + 2 );

    if ( terminalCommand != NULL )
    {    
        strcpy( terminalCommand, command );
        strcat( terminalCommand, xsdName );
        strcat( terminalCommand, " " );
        strcat( terminalCommand, xmlName );

        system( terminalCommand );

        free( terminalCommand );
    }
}

String concat in C

3 Answers3