Concatenating char arrays together

Question

This is something I would have considered trivial several years ago... It's been awhile since I've dabbled in C or C++ and I'm having an issue that is now causing a migraine.

I am receiving an error for the following code:

CompressFile::CompressFile(wchar_t *WorkingDirectory, wchar_t *File)
{
    int bzipError = BZ_OK;

    wprintf(L"Attempting to compress %s%s\n", WorkingDirectory, File);

    wchar_t FileRawInput[MAX_PATH];
    wcsncpy(FileRawInput, WorkingDirectory, sizeof(FileRawInput));
    wcsncat(FileRawInput, File, sizeof(FileRawInput));

    wchar_t bzipCompressedOutput[MAX_PATH];
    wcsncpy(bzipCompressedOutput, FileRawInput, sizeof(bzipCompressedOutput));
    wcscat(bzipCompressedOutput, L".bz2"); 

    wprintf(L"Output of string bzip: %s\n", bzipCompressedOutput);
    wprintf(L"Output of string raw: %s\n", FileRawInput);
}

I am receiving this the following error on line 8:

Unhandled exception at 0x64F4C6D1 in ias-agent.exe: 0xC00001A5: An invalid exception handler routine has been detected (parameters: 0x00000003).

I've already gone the distance to avoid using the string class, and I'd like to keep it that way for the time being. All I am trying to do, is add two strings together for RawFileInput and then add the value of RawFileInput to bzipCompressionOutput and finally, concatenate .bz2 to the end of bzipCompressionOutput.

If you want to avoid using C++, then you can always tag this question as C — KABoissonneault, May 25 '16 at 13:55
If you need a modifiable `wchar` buffer, use `std::vector`, otherwise, use `std::wstring`. This C-style string business is a waste of your precious time. (As always, unless you can proof a significant advantage, but that seems unlikely in this case.) — Baum mit Augen, May 25 '16 at 13:57
The final parameter for `wcsncpy` and `wcsncat` should be the number of characters (i.e. `wchar_t`s) to copy, not the size measured in `char`s. — molbdnilo, May 25 '16 at 14:03
You tagged this as C++. Compare what is posted to [this](http://rextester.com/RKSUIT62212). No buffer overruns, no fiddling around with `sizeof` to mess things up, and is guaranteed to work correctly. — PaulMcKenzie, May 25 '16 at 14:07
A number of issues: first, you're calling `wcsncpy` and passing `sizeof(...)` which gives you the number of *bytes* in the buffer and which is wrong: the function takes the number of *characters* in the buffer, so you want `sizeof(x) / sizeof(x[0])`. Second, this smells like stack corruption to me, so I'll ask the obvious: are you 100% sure that `WorkingDirectory` and `File` fit into `MAX_PATH` characters? If they don't, `wcsncpy` will not null-terminate `FileRawInput` which could cause "weird" crashes. You should avoid `wcsncpy` like the plague... — Nik Bougalis, May 25 '16 at 14:09

score 2 · Accepted Answer · edited May 23 '17 at 12:22

2

In the last page of chapter 4 in his book: "The C++ Programming Language" Bjarne Stroustrup the creator of C++ says:

Prefer strings over C-style strings

It's only advice but I'd encourage you to follow it.

But your real problem is that you're stomping memory there are not sizeof(FileRawInput) wchar_ts in your FileRawInput likewise there are not sizeof(bzipCompressedOutput) in bzipCompressedOutput array, there are MAX_PATH wchar_ts in both. The problem is that sizeof will tell you the number of bytes in your array, but if each element is larger than 1 byte then you have incorrectly told wcsncpy and wscncat your character count. A wchar_t is generally 2 bytes: https://msdn.microsoft.com/en-us/library/s3f49ktz.aspx Meaning you're calling effectively wcsncpy(FileRawInput, WorkingDirectory, 200). Stomping memory 100 wchar_ts beyond what you have allocated. Correcting this will remove your segfault.

But in order to print a wide string you'll need to correctly use the %ls modifier to wprintf.

Ultimately your code should look something like this:

wprintf(L"Attempting to compress %ls%ls\n", WorkingDirectory, File);

wchar_t FileRawInput[MAX_PATH];
wcsncpy(FileRawInput, WorkingDirectory, MAX_PATH);
wcsncat(FileRawInput, File, MAX_PATH);

wchar_t bzipCompressedOutput[MAX_PATH];
wcsncpy(bzipCompressedOutput, FileRawInput, MAX_PATH);
wcscat(bzipCompressedOutput, L".bz2");

wprintf(L"Output of string bzip: %ls\n", bzipCompressedOutput);
wprintf(L"Output of string raw: %ls\n", FileRawInput);

Live Example

EDIT:

The OP has acquiesced to Bjarne Stroustrup's advice and gone to wstring: Concatenating char arrays together But for anyone else who is still insistent on using these C-Style functions, MAX_PATH must be large enough to accommodate wsclen(WorkingDirectory) + wsclen(File) + wsclen(L".bz2") plus the L'\0' character, so perhaps placing a if statement on this function would be useful or perhaps:

assert(MAX_PATH > wsclen(WorkingDirectory) + wsclen(File) + wsclen(L".bz2"))

edited May 23 '17 at 12:22

Community

1
1

answered May 25 '16 at 14:30

Jonathan Mee

37,899
23
129
288

1

I did fix my issue... but I took the suggestions above to move to strings. Heck, if the man himself is telling you to use strings, then you probably should. Thanks for taking the time! – user0000001 May 25 '16 at 14:33
1

`sizeof(array)` is not the same as `sizeof(*)`. However, it does measure in bytes rather than units. – rici May 25 '16 at 14:33
@rici I intended the `*` there to be either `FileRawInput` or `bzipCompressedOutput` but I see that was unclear. I have edited out my laziness. – Jonathan Mee May 25 '16 at 14:48
Cool, +1. You missed the fact that the n in wcsn{cpy,cat} measure the source, not the destination, and that the destination needs to be one character longer for the NUL. As I suppose you probably know, `"%s"` in Microsoft's `wprintf` does expect a const wchar_t*`, which differs from standard C where it would expect a `const char*` pointing at a multibyte string. So `"%ls"` is required with a standards-compliant standard library, and thus for portability, but on windows `"%s"` will, unfortunately, do what OP expects. – rici May 25 '16 at 15:39
@rici I had assumed that `MAX_PATH` had accounted for that (maybe I should add a note on that.) The reason for the segfault was not because of insufficient space to write `FileRawInput` but insufficient space to write `sizeof(bzipCompressedOutput)` `wchar_t`s to `bzipCompressedOutput` when [`wcsncpy`](http://en.cppreference.com/w/cpp/string/wide/wcsncpy) executes: "If, after copying the terminating null wide character from `src`, `count` is not reached, additional null wide characters are written to `dest` until the total of `count` characters have been written." – Jonathan Mee May 25 '16 at 15:49
@JonathanMee: Indeed that was the immediate problem. But even if that were fixed, providing the precise size of the output buffer as the argument to *ncpy leads to the possibility that the result is not NUL-terminated. The only safe way to do it is to allocate one extra character, which you explicitly initialize to NUL. *ncat, on the other hand, guarantees NUL-termination but since you supply the size of the source, it's your responsibility to ensure that the dest has room for the NUL. These functions are hard to use, and just adding the `n` doesn't make your program safe. On the contrary. – rici May 25 '16 at 16:13
1

@rici I agree, this is why we use `wstring`s right? In the interest of completion I have appended the need for this check to the answer. – Jonathan Mee May 25 '16 at 16:28

Concatenating char arrays together

1 Answers1