4

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.

6
  • 2
    isn't it supposed to be strlen()? not strlent()? Feb 27, 2010 at 17:23
  • 1. String termination is '\0' and '\n' means newline. 2.objective of the function - append newline to given string. Whereas in the above function, input string is unchanged and a new string is returned.
    – N 1.1
    Feb 27, 2010 at 17:31
  • @nvl: how will you append a new line without doing a copy to another string ?
    – bragboy
    Feb 27, 2010 at 17:35
  • 1
    @Mark Byers: Maybe it checks whether the string contains a meat product and returns whether it's okay during Lent or not? Feb 27, 2010 at 17:37
  • @Bragaadeesh: since you are passing a pointer to string, you can just append '\n' (before the '\0') provided it has enough space. First, find the position where '\0' is before, assign '\n' to it and assign '\0' to the next position.
    – N 1.1
    Feb 27, 2010 at 17:41

13 Answers 13

9

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

7
  • Thanks that was a comprehensive explanation!
    – bragboy
    Feb 27, 2010 at 17:38
  • 1
    @rogue, where do you get that mistaken impression from? C defines the concept of local variables in a function. The standard implementation (common to practically every single platform, including x86) is that local variables are defined on the stack. Feb 27, 2010 at 17:56
  • 3
    @Daniel Goldberg: Your second point is incorrect. There's nothing wrong there. The code is pefectly valid C89/90. The previous "line of code" was a declaration of len with an initializer. C89/90 prohibits mixing declarations and statements ("code") inside a block. But as long as you just write contiguous declarations, you can put any "code" into the initializers. Feb 27, 2010 at 17:59
  • C has both stack and heap memory Feb 27, 2010 at 18:00
  • Where is it defined that C has a stack?
    – user1831086
    Feb 27, 2010 at 18:04
7

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.

4
  • I'm having trouble with this statement "'\n' is a new-line in c, just like in Java". I thought the proper way to refer to the newline in Java is through the line.separator property, because it's system-dependent. My understanding is that \n is LF, and \r is CR, but neither truly is the newline. Feb 27, 2010 at 17:39
  • depends on system. On windows its '\r\n', whereas on *nix, its '\n'.
    – N 1.1
    Feb 27, 2010 at 17:58
  • So it is erroneous to claim that "\n is a newline in C, just like in Java", right? Feb 27, 2010 at 18:02
  • @polygenelubricants:yes and no. He said he was using \n for a new-line in Java, and my point was that there's not any real reason to use something else in C. It is true that his terminology isn't exactly right wrt Java, but it doesn't really have anything to do with the question at hand, so I didn't worry about it... Feb 27, 2010 at 18:09
5

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.

1
  • strlent was a typo. Good catch on the local variable thing. Since I work more on java, i missed this point. Thanks!
    – bragboy
    Feb 27, 2010 at 17:36
3

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.

3

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;
}

2
  • I'm not sure this is a safe answer. What if the string does not have the extra char that you require? You'd have a one byte over-write. Feb 27, 2010 at 17:59
  • @Daniel Goldberg: read the edit. i think its pretty safe because the objective itself is to append :)
    – N 1.1
    Feb 27, 2010 at 18:06
2

Add a null after the newline:

buffer[len] = '\n';
buffer[len + 1] = 0;
2
  • Don't use 0 to set end of line '\0' is clearest in the code ! The same for the pointer don't use 0, use NULL ! Doesn't change anything but it makes your code more readable. Feb 27, 2010 at 17:27
  • That is an extremely old argument in the C programming world, and I respectfully disagree. To use the constant 0 is perfectly clear based on well-defined language semantics.
    – Pointy
    Feb 27, 2010 at 18:54
2

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

2

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.

1

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.
3
  • -1 because he said that buffer overflow is not a concern and because the newline character in fact is \n on all platforms. Streams convert it on platforms that use something else (Windows). You also miss the actual problems with the code.
    – Tronic
    Feb 27, 2010 at 17:29
  • @Tronic, the actual problems are? He's pointing out most of the bugs (and potential ones) and stack overflows should never be ignored. Feb 27, 2010 at 17:32
  • My understanding is that \n is LF, and \r is CR, but neither truly is the newline. If the function name is appendLF, then I wouldn't have any problem with it. Feb 27, 2010 at 17:43
1
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

1

I am ignoring the return of a local, as others have eloquently addressed that.

int len = strlen(str);
char buffer[1024];
...
buffer[len] = '\n';

If strlen(str) > 1024, then this sequence would write beyond the bounds of the declared buffer. Also as noted, this would (probably) not be null terminated.

To safely append a new line if possble,

char buffer[1024];
strncpy(buffer, str, 1024); // truncate string if it is too long
int len = strlen(buffer);
if (len < 1022) {
   buffer[len] = '\n';
   buffer[len + 1] = '\0';
} 

Note: If the string is too long, This leave the truncated string WITHOUT the new line.

0

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));
4
  • Thanks. Shouldnt we type cast the assignment to (char * )? - Because I remember malloc return type is void *
    – bragboy
    Feb 27, 2010 at 17:41
  • Yes, you should. Mostly as a good practice for further C coding, where your allocations and usage of memory can be far apart in code. Feb 27, 2010 at 18:00
  • @Bragaadesh @Daniel Goldberg: During assignment, pointer is typecast(ed) automatically. so char *p = malloc(1) will automatically make p a pointer to char. But, it is advisable and almost necessary to typecast yourself, following the standards.
    – N 1.1
    Feb 27, 2010 at 18:26
  • @nvl: Explicit casting is not required by ANSI C standard. You are allowed to do it, but it may suppress some useful compiler warnings. See: c-faq.com/malloc/mallocnocast.html
    – zoli2k
    Feb 28, 2010 at 12:27
-2

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.