don't use strxxx_s on Windows

[philippe44]

The use of these fails as 2nd argument is the same as 4th argument. Reading the cpp_reference defintions of these, it can't be right

[AlaricSenat]

Thank you for taking the time to upstream this! I completely forgot about it my bad...

I think you could just remove those defines, the commit body is enough as a justification and it's unlikely we re-implement them that way.

We will work to re-implement those wrappers properly for the next release, but for now, I'm in favor of merging a simple revert such as this one and release 1.14.14 ASAP! :D

[Vollstrecker]

Sorry, but If 0 is Always (no exceptions) the dumbest possible solution. That's the reason for gigabytes of dead Code transferred out there.

Just disabling this will Bring Back hundreds of warnings that clutter the compile output. A Clean solution would either be changing the macros to do the right transformations, or to write a little function that doed this and redirect to there. Everything else would just be a step Back, I've seen way too many commits just reverted Things that are problematic sometimes to a state that ist less problematic but Always, but git after that never really fixed because the reverter was satisfied enough and noone else cared.

As you Seen to User this in Win (the first after the Long period when this wasn't possible) I guess you can find a better solution.

[Vollstrecker]

Oh, and as our Tests (obviously) don't reveal this Problem and you have code that does, a test would bei very welcome, too.

[philippe44]

I know #if 0 is the dumbest solution, it was not supposed to be a long term fix, just an emergency change, per initial comment/request

But also back to the original comment, the easy fix is to use existing args and add count+1 as the 4th arg which totally defeat the purpose of using safe functions.

The issue happens when you use the following code

size_t size = strlen(src)+1;
char *dst = (char*) malloc(size);
strncpy(dst, src, size);

There is no good solution to that problem except changing all the calls to strncpy because the old behavior is faulty. You should use strcpy and not strcpy here because you know it works. So it's all the code ...

If I may, the best solution is to block VS to emit that warning on deprecated str function for now. This is what I do usually.

[Vollstrecker]

As I don't think this little fix justifies a new release (and then another one when it's really fixed), implementing the real fix will Help more. For a quick workaround you can add POSIX_OVERWRTIES_H to the defines when you configure the project and disable all the overwrites until it's done.

[mrjimenez]

Let me try to summarize, correct me if I am wrong:

Current Situation

• Library is faulty.
• No warning messages.

With this patch

• Library is ok, but there are security concerns.
• Full of compiler warning messages due to function deprecation.

If the previous summary is correct, I am in favor of accepting a modified version of this patch with some explanations in the code, while at the same time disabling the deprecation warning messages in windows.

Maybe we can put both the textual explanation and a pragma disabling the warning at the same place in the code, can't we? That way we could leave the #if 0 as a documented work in progress.

Also, I think a new release is urgent since the code is faulty.

Regards, Marcelo.

[Vollstrecker]

if the previous summary is correct,

It is.

That way we could leave the #if 0 as a documented work in progress.

I'm looking Forward to See how long this progress Takes (at least for the First try of a fix). From my side: I won't try as Long as I can't see something to reproduce the problem.

[mrjimenez]

I'm looking Forward to See how long this progress Takes (at least for the First try of a fix). From my side: I won't try as Long as I can't see something to reproduce the problem.

I would not worry about how long it will take. It is a step forward, since we avoid repeating the same mistake by documenting it.

If I understand correctly, the fact that there is not a concrete crash is bothering you, right? Well, the fact that the code is currently obviously wrong is also bothering me. Reverting it leaves us both satisfied :laughing:

@philippe44 , being practical:

1. Can you add some comments near the #if 0 resuming this discussion?
2. Can you add a #pragma GCC diagnostic ignored "-Wwhatever_diagnostic_message_is_spamming_compilation"?

Regards, Marcelo.

Regards, Marcelo.

[Vollstrecker]

Not really, it's just who should try to fix ist if noone can See the Problem (or if it is fixed). If I don't See that, I can't even say "you're using it wrong".

As stated, there is just an off-by-one on one the the 2 params, with an example to check I could fix this in sunday.

[AlaricSenat]

The current issues with those 4 defines:

• strncpy redefinition is wrong
• Theses functions assume they are being called on C arrays only. Using countof on a heap allocated pointer is undefined behavior and sizeof will only return the byte size of the pointer.

The reason we can't pin-point the places where it fails is because *_s fonctions have a significantly different behaviour than the replaced functions and have actual error returns values that are simply ignored here, leading to numerous unseen regressions. A first step could be to actually crash or log on _s failures to detect the potentials overflows or bad usages of the wrappers.

[mrjimenez]

With @AlaricSenat 's description, we only need someone to test the pragma to hide the compilation messages.

[philippe44]

Just to answer @Vollstrecker question about an example, just take that most simple one

#include <stdio.h>
#include "upnp.h"

#pragma comment(lib, "wsock32.lib")
#pragma comment(lib, "ws2_32.lib")
#pragma comment(lib, "iphlpapi.lib")

int handler(Upnp_EventType eventType, const void* event, void* cookie) {
    static int count;
    printf("Got an event %d\n", count++);
    return 1;
}

int main()
{
    UpnpClient_Handle handle;

    int rc = UpnpInit2(NULL, 0);
    rc = UpnpRegisterClient(handler, NULL, &handle);
    while (1) {
        Sleep(1);
    }
}

Compiled with static pupnp & pthreads, in debug mode. Let in run for a few seconds and it will crash when events are received in ssd_ctrlpt.c#159 where the location is copied because of the strncpy_s aliasing with args2 and args2 being equal

[mrjimenez]

I'm ok with the current state of the patch, does anyone wants to add something?

[AlaricSenat]

I'm in favor of these changes for 1.14.14 I'd just recommend to squash the three first commits together as they are simply modifications of the first one.

[mrjimenez]

I'm in favor of these changes for 1.14.14 I'd just recommend to squash the three first commits together as they are simply modifications of the first one.

I'd rather keep them separate since they are all addressing different stuff.

[AlaricSenat]

Thanks a lot, might be worth tagging a new release with the new fixes when you have time!

[mrjimenez]

Thanks a lot, might be worth tagging a new release with the new fixes when you have time!

Sure, it is on my list, just have to stop other stuff and do it carefully otherwise I tend to mess it up.

Regards, Marcelo.