Microsoft interview Question: Find the errors in this function

Question

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

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

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

Thank you all.

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.

Appreciate your time.

Answer 1

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

Answer 2

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

Answer 3

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

Answer 4

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

Answer 5

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

Answer 6

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

Answer 7

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
}

Answer 8

For c-style string it may be

char* // we want return a mutable string? OK
AddNewlineToString(
  const char* s // we don't need to change original string, so it's const
)
{
     const size_t MAX_SIZE = 1024; // if it's mutable string, 
                                   // it should have 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,
                      // user will check result and make a decision -
                      // to log, raise exception, exit(), etc

     // static                    // we want thread-safe result
     char* buf = new char[1024];  // so we allocate memory in 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")); // compiler will do it in one action like
        // *(int16_t*)(buf + len) = *(int16_t*)"\n";
        // rather then two assigns

     return buf;
}

Answer 9

I would leave the Job Interview ....

Answer 10

No need for return pointer. Change the incoming pointer.

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

Answer 11

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.

Answer 12

Can be made very simple using strdup:

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

Answer 13

Here goes a simpler way:

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