What is wrong with this function?

Question

I got a problem today. It had a method and I need to find the problem in that function. The objective of the function is to append new line to the string that is passed. Following is the code

char* appendNewLine(char* str){
    int len = strlen(str);
    char buffer[1024];
    strcpy(buffer, str);
    buffer[len] = '\n';
    return buffer;
}

I had identified the problem with this method. Its kind of straight forward. The method is having a potential of having array's index out of range. That is not my doubt. In java, I use '\n' for newline. (I am basically a Java programmer, its been many years I've worked in C). But I vaguely remember '\n' is to denote termination for a string in C. Is that also a problem with this program?

Please advise.

Answer 1

Add a null after the newline:

buffer[len] = '\n';
buffer[len + 1] = 0;

Answer 2

The terminator for a string in C is '\0' not '\n'. It stands only for newline.

Answer 3

First this is a function, not a program.

This function returns a pointer to a local variable. Such variables are typically created on the stack are no more available when the function exits.

Another problem is if the passed is longer than 1024 chars ; in this case, strcpy() will write past the buffer. One solution is to allocate a new buffer in dynamic memory and to return a pointer to that buffer. The size of the buffer shall be len +2 (all chars + newline + \0 string terminator), but someone will have to free this buffer (and possibly the initial buffer as well).

strlent() does not exist, it should be strlen() but I suppose this is just a typo.

Answer 4

No, a '\n' is a new-line in c, just like in Java (Java grabbed that from C). You've identified one problem: if the input string is longer than your buffer, you'll write past the end of buffer. Worse, your return buffer; returns the address of memory that's local to the function and will cease to exist when the function exits.

Answer 5

There are several issues with the code:

• It can buffer overflow since buffer is hardcoded to allocate only 1024 characters. Worse yet, the buffer is not even allocated in the heap.
• The newline "character" is actually operating system-dependent. Strictly speaking, it's only \n in Unix etc. In Windows, and in strict internet protocol, it's \r\n, for example.
• The string returned by the function is not null-terminated. This is most likely not what you'd want.

---

Also, taking into account your background in Java, here are some things that you should consider:

• Since you're working with C char* and not (immutable) Java strings, maybe you could append the newline in-place?
• Array access is no longer checked at run time, so you have to be VERY careful about going out of bounds. Make sure that all buffers are of appropriate size.
• The language does not come with standard automatic garbage collection, so if you do choose to allocate new buffers for string manipulation, make sure that you manage your memory properly and aren't leaking everywhere.

Answer 6

This function returns buffer, which is a local variable on the stack. As soon as the function returns the memory for buffer can be reused for another purpose.

You need to allocate memory using malloc or similar if you intend to return it from a function.

There are other issues with the code as well - you do not ensure that buffer is large enough to contain the string you are trying to copy to it and you do not make sure the string ends with a null-terminator.

Answer 7

There are at least two problems with your program.

Firstly, you seem to want to build a string, but you never zero-terminate it.

Secondly, you function returns a pointer to locally declared buffer. Doing this makes no sense.

Answer 8

Theres quite a few problems in this code.

1. strlen and not strlent, unless you have an odd library function there.
2. You're defining a static buffer on the stack. This is a potential bug (and a security one as well) since a line later, you're copying the string to it without checking for length. Possible solutions to that can either be allocating the memory on the heap (with a combination of strlen and malloc), or using strncpy and accepting the cut off of the string.
3. Appending '\n' indeed solves the problem of adding a new line, but this creates a further bug in that the string is currently not null terminated. Solution: Append '\n' and '\0' to null terminate the new string.
4. As others have mentioned, you're returning a pointer to a local variable, this is a severe bug and makes the return value corrupt within a short time.

To expand your understanding of these problems, please look up what C-style strings are, potentially from here. Also, teach yourself the difference between variables allocated on the stack and variables allocated on the heap.

EDITed: AndreyT is correct, the definition of length is valid

Answer 9

C string must end with '\0'.

buffer[len+1] = '\0';

You should dynamically allocate the buffer as a pointer to char of size len:

char *buffer = malloc(len*sizeof(char));

Answer 10

Maybe \n should be \r\n. Return + new line. It's what i always use and works for me.

Answer 11

C strings end with '\0'.
And as your objective is to append newLine, following would do fine (will save you copying the entire string into a buffer):

char* appendNewLine(char* str){
    while(*str != '\0') str++;  //assumming the string ended with '\0'
    *str++ = '\n';  //assign and increment the pointer
    *str = '\0';
    return str;  //optional, you could also send 0 or 1, whether 
                 //it was successful or not
}

EDIT :
String should have space to accommodate the extra '\n' and since the OBJECTIVE itself is to append, which means adding to the original, its safe to assume string has space for atleast one more char!!
But, if you dont want to assume anything,

char* appendNewLine(char* str){
    int length = strlen(str);
    char *newStr = (char *)malloc(1 + length);
    *(newStr + length) = '\n';
    *(newStr + length + 1) = '\0';
    return newStr;
}

Answer 12

char* appendNewLine(char* str){
    int len = strlen(str);
    char buffer[1024];
    strcpy(buffer, str);
    buffer[len] = '\n';
    return buffer;
}

Another important issue is the buffer variable; its supposed to be a local stack variable. As soon as the function returns it is being destroyed from stack. And returning pointer to the buffer probably means you are going to crash your process if you try to write at the returned pointer (address of buffer that's address on stack).

Use malloc instead