Fix ws binary/base64 subprotocol priority#595
Conversation
bk138
left a comment
bk138
left a comment
There was a problem hiding this comment.
Thanks for the PR! Some work left to do 😇
src/libvncserver/websockets.c
Outdated
| } | ||
|
|
||
| if ((protocol) && (strstr(protocol, "base64"))) { | ||
| proto_binary = strstr(protocol, "binary"); |
There was a problem hiding this comment.
I think that's UB if protocol is NULL. Maybe just inline those calls?
| if ((protocol) && (strstr(protocol, "base64"))) { | ||
| proto_binary = strstr(protocol, "binary"); | ||
| proto_base64 = strstr(protocol, "base64"); | ||
| if ((protocol) && (proto_base64 && (!proto_binary || proto_base64 < proto_binary))) { |
There was a problem hiding this comment.
After the ||, you're comparing char*? That can't be right.
There was a problem hiding this comment.
Comparing pointers is the priority check: the one that comes first has the priority.
| return FALSE; | ||
| } | ||
|
|
||
| if ((protocol) && (strstr(protocol, "base64"))) { |
There was a problem hiding this comment.
All in all: why not simply swap the if and else blocks?
There was a problem hiding this comment.
Some articles mention the list of protocols like "a list of sub-protocols that the client can handle in the priority order". So it's logical to respect the priority.
VelorumS
force-pushed
the
ws-subproto-priority
branch
from
fe6607c to
76a0f50
Compare
|
@VelorumS: Have you progressed on your PR? |
|
@Neustradamus I've responded to the review, haven't heard anything since. |
3 participants
The problem is when the client advertises
"protocol": ["binary", "base64"], the server picksbase64. The server should prefer the first one.It's an issue when the client had this configuration as a fix to the
400 Client must support 'binary' or 'base64' protocolerror with noVNC and old websockify + VNC: novnc/noVNC#1276 (comment) . Today it seems that noVNC doesn't dobase64any more. Although it's now incorrect for the client to advertise both protocols, it's not intuitive that"protocol": ["binary", "base64"]doesn't work while"protocol": ["binary"]does.Relevant PR/issue:
#342
novnc/noVNC#1310