What are the errors in this function from a Microsoft interview question?

I was asked this question in the MS written walkin-interview:

Find errors in the program below which is supposed to return a new string with \n appended to it.

char* Addnew开发者_如何学ClinetoString(char *s)
{
  char buffer[1024];
  strcpy(buffer,s);
  buffer[strlen(s)-1] = '\n';
  return buffer;
}

I'd tried to code on myself and was able to get it working by making buffer variable global and having buffer[strlen(s)] = '\n'. But did not know there were many other bugs in it.

I can see a few:

Length of input string not checked.

What if the strlen(s) > 1023? You can fit a string of length at most 1023 in buffer.

Overwriting the last char with \n

You are overwriting last char with newline. Your \n should go where \0 used to be and you need to add a new \0 after \n

Variable buffer is local to function and you are returning its address.

Memory for buffer is allocated on stack and once the function returns, that memory is freed.

I would do:

char* AddnewlinetoString(char *s) {

  size_t buffLen = strlen(s) + 2; // +1 for '\n' +1 for '\0'
  char *buffer = malloc(buffLen); 
  if(!buffer) {
   fprintf(stderr,"Error allocting\n");
   exit(1);
  }
  strcpy(buffer,s);
  buffer[buffLen-2] = '\n';
  buffer[buffLen-1] = 0;
  return buffer;
}

1. there's no limit in strcpy, better use strncpy.
2. you are copying to static buffer and returning pointer.

Here is a C++ version without errors:

std::string AddnewlinetoString(std::string const& s)
{
    return s + "\n";
}

And here is how I would probably write that in C++0x:

std::string AddnewlinetoString(std::string s)
{
    return std::move(s += "\n");
}

I would also add that the name of the method should stick to pattern and each word should start with capital letter:

char* AddNewlineToString(char *s)
{
}

ps. Thanks Konrad, I have changed the method name as you have suggested

3 things

   int len = strlen(s);
   char* buffer = (char*) malloc (len + 2);   // 1
   strcpy(buffer,s);
   buffer[len] = '\n';           // 2 
   buffer[len+1] = '\0';         // 3
   return buffer;

Edit: Based on comments

Here is a corrected version (community wiki incase I missed anything)

// caller must free() returned buffer string!
char* AddnewlinetoString(char *s)
{
  size_t len;
  char * buffer;

  if (s == NULL)
    s = "";

  len = strlen(s);
  buffer = malloc(len+2);
  if (buffer == NULL)
    abort();
  strcpy(buffer,s);
  buffer[len] = '\n';
  buffer[len+1] = 0;
  return buffer;
}

As tony mentions, s may be a valid address but still be a malformed c-string, with no null bytes. The function could end up reading until it causes a segfault, or some other horrible thing. While this is still idiomatic C, most folks prefer counted strings (rather than null terminated ones.)

// caller must free() returned buffer string!
char* AddnewlinetoStringN(char *s, size_t len)
{
  char * buffer;

  if (s == NULL)
    s = "";

  buffer = malloc(len+1); // only add 1 byte, since there's no need for the nul
  if (buffer == NULL)
    abort();
  strncpy(buffer,s,len);
  buffer[len] = '\n';
  return buffer;
}

The main problem with this code is that it's vulnerable to a stack buffer overflow exploit. It's a classic example.

Basically, the input char* can be made longer than 1024 bytes; these extra bytes will then overwrite the stack, allowing an attacker to modify the function return pointer to point to their malicious code. Your program will then unwittingly execute the malicious code.

Microsoft might be expected to care a good deal about these kinds of exploits, since the Code Red Worm famously used a stack buffer overflow to attack hundreds of thousands of computers running IIS web server software in 2001.

No need for return pointer. Change the incoming pointer.

int len = strlen(s); s[len] = '\n'; s[len + 1] = '\0';

In C++ it should be

std::string AddNewlineToString(const std::string& s) // pass by const reference
{
    return s + '\n'; // and let optimizer optimize memory allocations
}

Can be made very simple using strdup:

char* AddnewlinetoString(char *s) {
char *buffer = strdup(s);
buffer[strlen(s)-1] = '\n';
return buffer;
}

For C-style strings it may be

char* // we want return a mutable string? OK
AddNewlineToString(
  const char* s // We don't need to change the original string, so it's const.
)
{
     const size_t MAX_SIZE = 1024; // if it's a mutable string,
                                   // it should have a known capacity.

     size_t len = strlen(s);
     if(len + sizeof("\n") // To avoid the magic number "2".
         > MAX_SIZE)
         return NULL; // We don't know what to do with this situation,
                      // the user will check the result and make a decision -
                      // to log, raise exception, exit(), etc.

     // static                    // We want a thread-safe result
     char* buf = new char[1024];  // so we allocate memory in the heap
                                  // and it's C-language-string but not C language :)

     memcpy(buf, s, len); // Copy terminating zero, and rewrite it later? NO.
     memcpy(buf + len, "\n", sizeof("\n")); // The compiler will do it in one action like
        // *(int16_t*)(buf + len) = *(int16_t*)"\n";
        // rather than two assignments.

     return buf;
}

Here goes a simpler way:

char* AddnewlinetoString(char *s)
{
  char buffer[strlen(s)+1];
  snprintf(buffer,strlen(s),"%s\n",s);
  return buffer;
}