libwebsockets icon indicating copy to clipboard operation
libwebsockets copied to clipboard

Fixed missing CRLF

Open PoltoS opened this issue 9 months ago • 7 comments

Partially reverts https://github.com/warmcat/libwebsockets/commit/ef4a85d0e14270e83ce74213a0b4bb53e29a07be

Closes https://github.com/warmcat/libwebsockets/issues/3367

PoltoS avatar Apr 30 '25 19:04 PoltoS

Hi @lws-team , could you please check this fix. The reasoning is in #3367

PoltoS avatar Jun 14 '25 22:06 PoltoS

Yes I get it that something is up with this, however the code runs in different modes which act differently depending on if you have pm_deflate (and perhaps ws-over-h2) or not. I need to find the time to figure out the scope of it and try them all.

lws-team avatar Jun 15 '25 05:06 lws-team

Thanks @PoltoS and @lws-team, would be great if this gets fixed.

roxlu avatar Jul 27 '25 09:07 roxlu

I can't reproduce it, as told here: https://github.com/warmcat/libwebsockets/issues/3367#issuecomment-2973554721 Instructions for the case where there's a problem will help me solve it (whatever it is).

lws-team avatar Jul 27 '25 09:07 lws-team

@lws-team In #3367 I shared my compilation parameters that lead to the issue with the current code. Doesn't it work for you to reproduce the issue? If not, please share on which system do you work? Maybe you can add this combination in CI/CD to check?

PoltoS avatar Jul 27 '25 22:07 PoltoS

So there is a few disconnects here that have stopped any forward progress. Let me use up some more of my valuable time for free, to try to reconnect everyone who cares about this issue to find some common ground.

  1. This problem does not exist in the context of a user project, or platform. Whatever is or was broken, it will be broken everywhere. It's convenient for you to see it from a specific platform perspective because that is what is in front of you with the symptom. I don't have your platform, what we all have is the minimal examples. The expectation I, for free, will bend myself out of shape and spend time aligning to your situation for free is doomed. So the way forward is test against the minimal examples I spent a lot of time creating, so we can all test on the "same platform" at least for user code. Irksome! Annoying! Yes. But effective and something we can all do cheaply.

  2. It costs a lot of my personal time to add a platform in CI, not only is the idea to essentially cross-build with an android SDK but the bug requires it to be able to run on Android, so it means a simple app and then ADB, and then acquire logs and wire it up in CI. It's something like a man-month from cold. The expectation I will put myself through all that for free, so you can make money, is doomed. So I hope we can adjust expectations and listen when I ask for information based on the minimal examples preferably on linux.

  3. It can be seen in #3367 although there are at least two reporters, the second reporter seems to disagree that your patch solves the problem for him. I am the third "reporter" and in the case I described, which can be reproduced with the minimal examples, it works. If you accept that "it works for me" without this patch adding a CRLF, you must accept that adding your patch will break it for me. That is why your patch can't be accepted until the conditions that make the problem are understood in a way that all the reporters see makes sense. It is likely more complicated story underneath but at the end of the day, no maintainer will accept a patch that breaks his project for him.

  4. You may feel you have "shared [your] compilation parameters" but you have not, where is the cross file contents? That's going to be full of relevant cmake options. What exact version of lws are you seeing this on?

  5. This seems quite solveable there are three different and conflicting claims people are willing to make and hopefully stand by: a) You: It [what version?] is broken in the way you describe and needs the patch. b) @mbicha68: it [ what version?] is broken some way but needs a different patch. c) Me: I tried unpatched main ./minimal-examples-lowlevel/ws-server/minimal-ws-server-pmd-bulk on Jun 15 with pmd extension enabled, and it worked, I showed a hexdump of what it emitted. The way forward is do the same on current main or v4.4-stable, and show me how to build that example on linux so it breaks in the way you have been experiencing, or align your android build to my setup that works.

lws-team avatar Jul 28 '25 04:07 lws-team

I have been working with Gemini 2.5, I showed it the related code and these issues. Its take is that in the case the extension is negotiated with the server, it works OK today unpatched. That's my test situation, the server supports the extension and negotiates it OK.

However in the case the extension is not negotiated, it's broken according to our new Overlords. It proposes this fix to keep it working in the accepted case and to solve the problem when the extension is not negotiated... does that work for you?

diff --git a/lib/roles/ws/server-ws.c b/lib/roles/ws/server-ws.c
index 82e3ed98..a356d183 100644
--- a/lib/roles/ws/server-ws.c
+++ b/lib/roles/ws/server-ws.c
@@ -737,9 +737,10 @@ handshake_0405(struct lws_context *context, struct lws *wsi)
 
                LWS_CPYAPP(p, "\x0d\x0aSec-WebSocket-Protocol: ");
                p += lws_snprintf(p, 128, "%s", prot);
-               LWS_CPYAPP(p, "\x0d\x0a");
        }
 
+       LWS_CPYAPP(p, "\x0d\x0a");
+
 #if !defined(LWS_WITHOUT_EXTENSIONS)
        /*
         * Figure out which extensions the client has that we want to

lws-team avatar Jul 28 '25 04:07 lws-team