strcat_s problems

I'm writing a function that combines first, middle, and last names into another array. Using strcat and strcpy in the following code, everything works fine (they obviously don't take the integer in the middle). Also, the definitions in the main program are:

char First[15], Middle[15], Last[15];

Here's the function that's giving me problems:

Code:

---

char * combineNames(const char *First, const char *Middle, const char *Last)
{
        char *FullName = new char[46];

        strcpy_s(FullName, 15*4, Last);
        strcat_s(FullName, 15*4, ", ");
        strcat_s(FullName, 15*4, First);
        strcat_s(FullName, 15*4, " ");
        strcat_s(FullName, 15*4, Middle);

        return FullName;
}

---

I'm really swimming on the integer to put in there. I tried smaller ones and got error messages (or crashes), and these were just my last experimental values before turning to you guys. The message I get when I run the program goes something like: "application wrote to memory after end of heap buffer" and it gives a buffer location, too.

I also think the number in the middle is supposed to be a number of bytes rather than number of places in the array.

As I say, using strcpy and strcat with no integer, the whole thing works fine, but it looks like those are deprecated according to warnings in MS suggesting using strcpy_s instead.

just for clarity, this is the version that works (but gives warning messages when compiling):

Code:

---

char * combineNames(const char *First, const char *Middle, const char *Last)
{
        char *FullName = new char[46];

        strcpy(FullName, Last);
        strcat(FullName, ", ");
        strcat(FullName, First);
        strcat(FullName, " ");
        strcat(FullName, Middle);
/*       
        strcpy_s(FullName, 15*4, Last);
        strcat_s(FullName, 15*4, ", ");
        strcat_s(FullName, 15*4, First);
        strcat_s(FullName, 15*4, " ");
        strcat_s(FullName, 15*4, Middle);
*/
        return FullName;
}

---

Quote:

---

Originally Posted by Aisthesis

As I say, using strcpy and strcat with no integer, the whole thing works fine, but it looks like those are deprecated according to warnings in MS suggesting using strcpy_s instead.

---

Microsoft is deprecated. By that I mean that I do not recommend heeding Microsoft's statement that those functions are deprecated because they are not. Perhaps you can infer that Microsoft is not actually deprecated, but rather merely recommend their versions of these functions over the standard versions.

To put a spanner in the works, I suggest not using null terminated C style string variables. Use std::string instead, e.g.,

Code:

---

std::string combineNames(const std::string& First, const std::string& Middle, const std::string& Last)
{
    return Last + ", " + First + " " + Middle;
}

---

or if you want to avoid excessive concatenation:

Code:

---

std::string combineNames(const std::string& First, const std::string& Middle, const std::string& Last)
{
    std::stringstream ss;
    ss << Last << ", " << First << " " << Middle;
    return ss.str();
}

---

or if you can use Boost:

Code:

---

std::string combineNames(const std::string& First, const std::string& Middle, const std::string& Last)
{
    return str(boost::format("%3%, %1% %2%") % First % Middle % Last);
}

---

Quote:

---

Originally Posted by laserlight

Microsoft is deprecated. By that I mean that I do not recommend heeding Microsoft's statement that those functions are deprecated because they are not. Perhaps you can infer that Microsoft is not actually deprecated, but rather merely recommend their versions of these functions over the standard versions.

---

I would recommend strcpy_s and strcat_s over the normal ones because portability can be achieved with a simple macro.
As for why it doesn't work (aside from the real solution you really should be using that laserlight just mentioned), it's because you are writing beyond the buffer.
The first doesn't work because 1) your buffer sizes are WRONG (you allocate 46 chars, yet say you have 60) and 2) because you ARE writing more data than available and the functions are trying to tell you that.
The second doesn't work because of reason 2 above. It doesn't give you any helpful assert, but instead tells you that you clobbered (or overwrote) some memory (you shouldn't have).
All in all, undefined behavior and often a crasher and a security risk.

If you don't want to use std::string, you can determine how much space is needed.

Code:

---

unsigned TotalLength = strlen(First) + strlen(Middle) + strlen(Last)
    + 3 //", " + " "
    + 1; //terminator
char* FullName = new char[TotalLength];

---

Well, the assignment calls for using character arrays and not strings for First, Middle, Last and FullName.

As for the buffer overruns, I got an error also when I used

strcpy_s(FullName, 14*4, Last);
strcat_s(FullName, 2*4, ", ");
strcat_s(FullName, 14*4, First);
strcat_s(FullName, 1*4, " ");
strcat_s(FullName, 14*4, Middle);

I'm not certain whether it was the same message, but I do remember that that didn't work--similarly when I tried not counting the null terminator and added 1 to each, similarly when I didn't multiply by 4, which I started doing because I concluded that these were supposed to be bytes rather than places in the array--the reason I came up with 15 for all was because the small values that seemed like possible right ones didn't work, so I decided to just give everything plenty of headroom, and that didn't work either. But the real point is that I'm just blindly groping here for numbers that work without understanding the principle behind it.

So, if we restrict the problem to what numbers are required to make this work GIVEN the definitions of the arrays, what do we need to put in the middle field on each of these? And what's the general rule for figuring the number we need there?

As to the deprecated issue, I was surprized, too, as the book I'm working from was published this year, so it should be up-to-date on what's deprecated and what isn't. Book likes using strncat, strncpy in most cases, but I figured strcpy/strcat are fine here because the array definitions guarantee that bounds can't be overstepped.

Anyhow, I'm taking your comments to mean "don't worry about strcat/strcpy," which is fine, but I'd still like to understand the functions MS is recommending.

As to the length of the arrays, they are determined exactly by anon's equation. Note that with First[15] maximum strlen is going to be 14 (same for the others). So, we have 14 + 14 + 14 + 3 + 1 = 46. That's why I picked 46 for the length of FullName.

But that's not what's giving me problems. It's this mysterious integer that MS's function want and for which I don't understand how one is supposed to pick an appropriate value.

> It's this mysterious integer that MS's function want
So did you ever bother to read the manual?
strcat_s, wcscat_s, _mbscat_s

Code:

---

errno_t strcat_s(
  char *strDestination,
  size_t numberOfElements,
  const char *strSource
);

---

Quote:

---

Parameters

strDestination

Null-terminated destination string buffer.

numberOfElements

Size of the destination string buffer.

strSource

Null-terminated source string buffer.

Return Value

Zero if successful; an error code on failure.

---

Every call should be 46 (or whatever number you allocated)

Without the additional, non-standard "size" parametre, it's not any more secure than the standard version. Might as well just use the standard version.

Quote:

---

I would recommend strcpy_s and strcat_s over the normal ones because portability can be achieved with a simple macro.

---

If you can make it standard compliant by just using a macro, the way you are using it is not any more secure than the standard version. Might as well just use the standard version in the first place.

The way these _s (non-standard) functions work is that, they take an additional "buffer size" parametre, so they can make sure you won't overstep it. Calling them without the buffer size paramtre (is that even allowed?) is rather pointless.

[edit]
Of course, the same can be achieved using standard, portable functions by checking the length of the input before passing it to strcat.
[/edit]

Quote:

---

Originally Posted by cyberfish

If you can make it standard compliant by just using a macro, the way you are using it is not any more secure than the standard version.

---

I believe Elysia's idea is to conditionally define macros that forward the extended versions to the standard versions if the extended versions are not provided.

Quote:

---

Originally Posted by cyberfish

The way these _s (non-standard) functions work is that, they take an additional "buffer size" parametre, so they can make sure you won't overstep it.

---

Not quite: they make sure that if there is out of bounds access, it will be detected.

Quote:

---

I believe Elysia's idea is to conditionally define macros that forward the extended versions to the standard versions if the extended versions are not provided.

---

But that means, when compiled by a compiler without those extensions (for example, every compiler other than MSVC), the code won't be secure, assuming the programmer relies on the bound-checking extension.

And then there are functions that have different signatures than the standard equivalents. Like fopen_s

fopen_s, _wfopen_s

As far as I can see, it makes the return value of the regular fopen a parametre (pointer to pointer), which allows it to return a more detailed error (rather than just a null pointer for all errors).

Whether that really helps I can't tell, certainly not in terms of "security", though, but at most in convenience. Microsoft is just specializing C.

Not so easy to fix with macros.

And then there is scanf_s, which takes a buffer size for all "%s" parametres. Not so easy to fix with a macro either, and there is a perfectly standard and portable way to do it (just include the field width with the placeholders - "%10s"). But no, they deprecated scanf and made a new, non-compatible one.

And then there's gets(). I agree it should be deprecated, but not for gets_s. Why not just deprecate it in favour of fgets?

While some _s functions are probably better (in some way), than the standard functions, it gives me the impression that they are trying very hard to find any slightest reason to deprecate standard functions and replace them with their _s functions.

There's even a rand_s(). Same deal as fopen_s. Take in the return address as a pointer, just so it can return an error value. Exactly how many different ways can rand() fail? (I don't think they deprecated rand(), though, so that's good)

Quote:

---

Originally Posted by cyberfish

But that means, when compiled by a compiler without those extensions (for example, every compiler other than MSVC), the code won't be secure, assuming the programmer relies on the bound-checking extension.

---

When it comes down to it, just using those extended versions does not make the code secure anyway. It just helps to detect lapses. Relying on the bounds checking would be like relying on std::out_of_range to be thrown when using the at() member function of std::vector, as opposed to making sure that the accesses are within range and just using the bounds checking provided to be on the safe side.

Quote:

---

Originally Posted by cyberfish

While some _s functions are probably better (in some way), than the standard functions, it gives me the impression that they are trying very hard to find any slightest reason to deprecate standard functions and replace them with their _s functions.

---

They are not deprecating the standard library functions, and in fact they have no power to do so. I repeat, when they say "deprecate", they mean "we do not recommend". To use their own language: their use of "deprecated" is deprecated.

Quote:

---

They are not deprecating the standard library functions, and in fact they have no power to do so. I repeat, when they say "deprecate", they mean "we do not recommend". To use their own language: their use of "deprecated" is deprecated.

---

That's good news!

"Deprecated" is a strong word IMHO. In GCC, a deprecated feature is one that may be removed in a later release. I certainly hope they are not going to remove std lib functions.

Still, they are "not recommending" many standard functions, and replacing them with their own, when there are standard ways to solve the "problems" they find with standard functions.

For a C newbie, this can cause pretty bad vendor lock-in. Most of them will think if Microsoft is deprecating a function, it's for good reasons that they don't understand, and follow it blindly. To make matters worse, Microsoft is carefully hiding the fact that those are non-portable functions. For example, if you look at the page for scanf_s,
scanf_s, _scanf_s_l, wscanf_s, _wscanf_s_l
There is absolutely NO mentioning that it is not a standard function, and can easily mislead, especially since it's part of <stdio.h>.

Maybe we should start recommending newbies to start with GCC, and only (optionally) switch to MSVC when they are good enough to make their own judgments and not be misled by MS propagandas like this.

It's soon going to be as bad as learning web designing with IE.

interesting the discussion on deprecation and IDEs.

to Salem: So, if the number is supposed to be the SIZE of the destination string buffer, and my destination string buffer is FullName[46], does that mean that I need to use 46 every time? like as first command

strcpy_s(FullName, 46, Last);

using 46 (size of FullName) the whole way through makes it work.

Quote:

---

Originally Posted by Aisthesis

interesting the discussion on deprecation and IDEs.

to Salem: So, if the number is supposed to be the SIZE of the destination string buffer, and my destination string buffer is FullName[46], does that mean that I need to use 46 every time? like as first command

strcpy_s(FullName, 46, Last);

---

Yes, that or better yet, sizeof(FullName).

I also agree that some functions cannot simply be wrapped in macros. They can be a little more problematic, and perhaps should be avoided by newbies, but strcat_s and strcpy_s are very, very easy using macros:

Code:

---

#ifndef _MSC_VER
#define strcpy_s(dst, dst_length, src) strcpy(dst, src)
#endif

---

And one should also remember that these so called "secure" functions may not be secure (misleading name), but they DO protect against programmer mistakes, such as out-of-bounds writing which is helpful to any programmer to aid in debugging and especially to newbies who can very easily make such mistakes.

So when compiling in MSVC, we get added debugging functionality (good), and if we compile in another compiler, it will still compile due to the macro wrapper, so it essentially becomes ISO standard code (good). So no harm comes at all.
This is what I base my suggestion at.
I might or might not suggest the same for other "safe" functions. And I do agree that there's a lot of broken documentation and a poor word of saying they aren't recommended.

Quote:

---

So when compiling in MSVC, we get added debugging functionality (good), and if we compile in another compiler, it will still compile due to the macro wrapper, so it essentially becomes ISO standard code (good). So no harm comes at all.

---

But the problem is, if the programmer relies on the buffer size checking function, the code, when compiled by another compiler, won't be secure (since the dst_length parametre is macro'ed out).

So the portable way would be to make sure the buffer size is big enough manually BEFORE calling strcpy(_s). At that point... might as well just call the standard function.

> Yes, that or better yet, sizeof(FullName).
Or not.
The OP was allocating memory, so sizeof isn't going to do what you want.

> but they DO protect against programmer mistakes,
But only if you're smart enough to RTFM and understand what it's telling you.
As the OP demonstrated in #6, if you LIE about the size of the buffer you're using, then you're no better off than you were before.

Except that you might get into a mind-set that so long as it doesn't actually crash, then your use of _s functions somehow makes it all nice and safe.

Quote:

---

Originally Posted by cyberfish

But the problem is, if the programmer relies on the buffer size checking function, the code, when compiled by another compiler, won't be secure (since the dst_length parametre is macro'ed out).

So the portable way would be to make sure the buffer size is big enough manually BEFORE calling strcpy(_s). At that point... might as well just call the standard function.

---

As I pointed out, it's not necessarily more secure at all. If you want to call an assert/crash more secure, then sure. Otherwise it's merely a debugging tool and not a security measure (then it would simply fail and return an error).
So for your second argument that becomes invalid too - because the *_s functions is a tool to help you catch mistakes where you didn't make your buffer big enough.
You really should make the buffers big enough in the first place, the "secure" functions won't fix that, but it will tell you if you screwed up.

Quote:

---

Originally Posted by Salem

> Yes, that or better yet, sizeof(FullName).
Or not.
The OP was allocating memory, so sizeof isn't going to do what you want.

---

True, when formulating that reply, I was looking at the...

Quote:

---

FullName[46]

---

...bit.
So if using a local buffer, ie char FullName[46], then sizeof will be right. If dynamically allocated via new, then it is not correct.

Quote:

---

> but they DO protect against programmer mistakes,
But only if you're smart enough to RTFM and understand what it's telling you.
As the OP demonstrated in #6, if you LIE about the size of the buffer you're using, then you're no better off than you were before.

---

Everything needs the knowledge to use it. Compiler warnings and errors and no different.
When you see a warning, you look it up and fix it. Normally anyway.
Though a newbie may not do this, so I do not see how it differs from that of compiler warning outputs.

Quote:

---

Except that you might get into a mind-set that so long as it doesn't actually crash, then your use of _s functions somehow makes it all nice and safe.

---

Ah yes, pitfalls. There are always those, and I don't think this is an exception.
And I don't think we should consider something bad because of a few pitfalls.
As with everything, it helps to understand the function and what it does and what it does not do.

*shrugs*
What else can I say?