Wrong `strncpy` to `strncpy_s` aliasing

[AlaricSenat]

6d3757549b9a8b938e9c1e5339d7e188a3c57911 Introduces critical regressions for windows.

While the intent is great, the implementation is wrong: #define strncpy(arg1,arg2,arg3) strncpy_s(arg1,arg3,arg2,arg3)

with the strncpy_s official prototype being:

errno_t strncpy_s(
   char *strDest,
   size_t numberOfElements,
   const char *strSource,
   size_t count
);

numberOfElements here is the same size as count. numberOfElements is supposed to include the zero terminator. Basically, if numberOfElements == count the function fails and leaves strDest empty.

The correct solution might be to implement strncpy_s for posix targets instead.

[Vollstrecker]

So basically this would be arg3+1 as the second arg. Or maybe better arg3-1 as the last one, as the size should be big enough to hold the terminator in addition to the string.

[AlaricSenat]

So basically this would be arg3+1 as the second arg. Or maybe better arg3-1 as the last one, as the size should be big enough to hold the terminator in addition to the string.

This could work but there's too many strncpy occurrences in the codebase for me to be 100% sure about that :(

Same issue with:

#define strcat(arg1, arg2) strcat_s(arg1, sizeof(arg1), arg2)

Every call to strcat() I've encountered in the code-base are called on a char * and not an array start so the sizeof() is hinting the wrong size.

I'm sorry but I think the approach here is flawed, either we:

• Assume the code-base does not contain any buffer overflows (probably wrong) and leave things as they were for Windows.
• Go through every str*() function calls to check whether they are correctly written in the first place and then eventually replace them by their str*_s() counterparts where it make sense to have the buffer empty and zeroed on overflow.

For now I suggest a revert of the new win32 #define section in ixml/inc/posix_overwrites.h in a new minor release of 1.14 because the latest one is just broken on windows targets :/

[philippe44]

Holy crap! I did not see your post until I found the issue myself. I went crazy over this one, not understanding why a simple example would crash as soon as there is a callback registered...

[mrjimenez]

A patch will be very well appreciated.

[philippe44]

@mrjimenez: sorry, my comment might be misleading. I just move from an antic 1.6 version to 1.14 and it was crashing after a few seconds of use even with the simples example when I was registering a callback. I was just mad at myself for not having looked into "issues" before wasting a lot of time accusing my build, compiler and many other things 😄

This https://github.com/pupnp/pupnp/pull/414 remove the problem, but I think who added that should step in to clarify the intent.

See here https://en.cppreference.com/w/c/string/byte/strncpy for the definition of the secured version

2) Same as (1), except that the function does not continue writing zeroes into the destination array to pad up to count, it stops after writing the terminating null character (if there was no null in the source, it writes one at dest[count] and then stops). Also, the following errors are detected at runtime and call the currently installed [constraint handler](https://en.cppreference.com/w/c/error/set_constraint_handler_s) function:
src or dest is a null pointer
destsz is zero or greater than RSIZE_MAX
count is greater than RSIZE_MAX
**count is greater or equal destsz, but destsz is less or equal strnlen_s(src, count), in other words, truncation would occur
overlap would occur between the source and the destination strings**

[mrjimenez]

Hi @philippe44 ,

No need to be sorry, my comment was not relative to yours. I understand there is a serious problem on windows, but I usually refrain to patch windows myself because I can't test it properly here.

I'll have a look at your pull request.

Best regards, Marcelo.