Let's see people from Java schools answer this one :)
–
LucasSep 21 '10 at 14:41

5

why this question has been closed? Its very much related to programming.
–
giriSep 21 '10 at 17:01

13

There are two kinds of programmers - the ones that will think through a problem to see all of the relevant details, and the ones who will just keep trying stuff until it seems to work. Guess which group you fall into? Guess which kind Microsoft is looking for?
–
Mark RansomSep 21 '10 at 17:49

Funnily enough, the \0 is probably still there (if the first error doesn't trip it). Look again, is the \0 overwritten?
–
Muhammad AlkarouriSep 21 '10 at 9:17

3

2. strlen doesn't include the terminator in the length. So at most, the code just replaces the last char of the string with a newline, but the nul will remain if it already was there.
–
cHaoSep 21 '10 at 9:18

3

@TygerKrash: The stack holds values local to the current function. Once the function returns, your pointer is pointing to garbage, i.e. you don't know what's there (e.g. the next function's local variables can overwrite the region your pointer points to).
–
3lectrologosSep 21 '10 at 11:32

6

This will leak memory if used like: str = AddNewlineToString(str);
–
sje397Sep 21 '10 at 14:08

4

@sje397: That's not a problem with the function, provided it is properly documented that the caller owns the returned buffer, and is responsible for freeing it appropriately. The only alternative is to work with a global buffer, which has much more severe issues. *edit: Given that you're stuck with a C-like interface, of course.
–
Dennis ZickefooseSep 21 '10 at 16:54

Instead of strncpy one should make sure the buffer is of suitable size. Exactly what good is it doing if you get the input truncated, rather than having newline appended? This should take care of both issues.
–
visitorSep 21 '10 at 9:16

If available, strlcpy would be better.
–
Matteo ItaliaSep 21 '10 at 9:22

True, but... I don't think the point of these exercises is to come up with a better version per se, but to understand the intricacies of the errors. Consider it a test of debugging your mediocre colleagues' work, rather than a test of writing new code yourself.
–
Andrzej DoyleSep 21 '10 at 9:25

The question is tagged C++, that would imply this is a potential correct answer!
–
AshleysBrainSep 21 '10 at 12:18

1

@Steve: I added your suggested reference to const version, but I still like the pass by value version better, especially with the std::move edited in :)
–
fredoverflowSep 21 '10 at 14:14

1

So in conclusion, I think the best way would be, for both C++03 and C++0x, std::string AddnewlinetoString(std::string s){ s += '\n'; return s; } Considering the parameter: in C++03, when working with an lvalue you'd make a copy anyway, so nothing is lost. And a compiler will elision the copy of an rvalue. In C++0x, you again make the needed copy, but are guaranteed to move-construct the parameter when it's given an rvalue. So nothing lost there. And then you do your manipulation, then return the copy. In C++03, you get NRVO and in C++0x, you get an implicit move return. (Again, I think.)
–
GManNickGSep 22 '10 at 18:11

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.)

there is just one more little hole.... if you pass in a string without a terminating zero, but to nail that down you would need to assume maximum lengths for strings.
–
AnthonyLambertSep 22 '10 at 10:18

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.

That need not work always. What if the input is a string literal or a char array which has len-1 char already stuffed in so that when you access index len+1 you are stepping past the allocated memory.
–
codaddictSep 23 '10 at 12:52

@codaddict: if it was a string literal, someone is ignoring the signature - it isn't a const parameter.
–
Jonathan LefflerDec 12 '10 at 4:20

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

Your use of sizeof("\n") was initially unclear; I'm not sure avoiding a "magic number" is worth it in this case.
–
Dennis ZickefooseSep 21 '10 at 16:43

What's the point of using new but not std::string?
–
GManNickGSep 22 '10 at 18:16

@Gman: for example if we want provide a C-compatible interface. With heap allocated memory, we should provide only one function to delete any heap-allocated memory, but for allocated-by-string memory we can't easily do it.
–
AbyxSep 22 '10 at 18:50

@Abyx: Pretty strange to justify something with "It might be used in C." I wouldn't assume that in any of my code, and such an assumption is certainly outside the scope of the question.
–
GManNickGSep 22 '10 at 19:08

@GMan: not "in C" but "C-compatible". It means "in C#, F#, Python (ctypes), Haskell and lots of other languages". You can't export function with std::string result from DLL, and use this function in module written in another language (or module compiled by another C++ compiler too). It's Microsoft's question, it's about Windows, not cross-platform programming with gcc only.
–
AbyxSep 22 '10 at 19:38