You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/04/23 14:00:59 UTC
[GitHub] [guacamole-server] holograph opened a new pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
holograph opened a new pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271
The can easily be tested by sending a well-formed `select` command using a `$`-prefixed, but fake, connection ID.
Before patch:
```
tomer@druuge:~/dev/guacamole-server$ echo '6.select,10.$123456789;' | nc localhost 4822
tomer@druuge:~/dev/guacamole-server$
```
Note complete lack of response above, the server simply drops the connection. Contrast with the same command after the patch:
```
tomer@druuge:~/dev/guacamole-server$ echo '6.select,10.$123456789;' | nc localhost 4822
5.error,38.Connection "$123456789" does not exist,3.768;
tomer@druuge:~/dev/guacamole-server$
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] holograph commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r771874510
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
Review comment:
Woah, it's been a while. I am no longer with my previous employer and have no real-world way of testing this (nor even a toolchain to build it with...). Hopefully it's still somewhat useful as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r413824810
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
+ guac_protocol_send_error(socket, message,
+ GUAC_PROTOCOL_STATUS_CLIENT_BAD_REQUEST);
Review comment:
In general I think this is probably a good thing to do, but, have you tested the mainstream Guacamole Client with these changes? Presumably you're developing a custom application that interacts directly with guacd, but we need to make sure these changes are compatible with the upstream client.
Also, I haven't looked, yet, but I'm guessing there may need to be some matching changes done within the guacamole-common-js code on the guacamole-client side of things to make sure errors are handled when send back by guacd? Or maybe this is already in place?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#issuecomment-997298342
Alrighty - superseded by #355.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] holograph commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r575815733
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
Review comment:
This isn't handled here - this code path is specifically for existing connections (i.e. selecting $<connection_id>)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper closed pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
mike-jumper closed pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r575726813
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
Review comment:
`guacd_log()` will interpret `message` here as a printf-style format string:
https://github.com/apache/guacamole-server/blob/ca1fbd5e980392dc91b52ddff473698035d8f357/src/guacd/log.h#L45-L49
To do this safely, the call would need to be:
```c
guacd_log(GUAC_LOG_INFO, "%s", message);
```
However, given that the connection is failing, I think `GUAC_LOG_ERROR` would be more appropriate.
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
+ guac_protocol_send_error(socket, message,
+ GUAC_PROTOCOL_STATUS_CLIENT_BAD_REQUEST);
Review comment:
I think `GUAC_PROTOCOL_STATUS_RESOURCE_NOT_FOUND` would be a better choice.
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
Review comment:
What about the case where a particular protocol is requested (for a new connection) and that protocol is not defined?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] holograph commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r575814820
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
Review comment:
Oh, I missed that -- so I can basically safely log a format string and skip constructing the message entirely? i.e.
```guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist", identifier);```
As for the level, this isn't a runtime error so much as a client error though, akin to HTTP 400 "bad request" or 404 "not found", which to me sounds more like an informative message if any; If you still think it's appropriate to log this an error, however, please ping me and I'll adjust the PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r771874140
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
Review comment:
> Oh, I missed that -- so I can basically safely log a format string and skip constructing the message entirely? i.e. `guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist", identifier);`
Yep!
> As for the level, this isn't a runtime error so much as a client error though, akin to HTTP 400 "bad request" or 404 "not found", which to me sounds more like an informative message if any; ...
Good point. I agree it makes sense for this to remain at the info level, with any further handling of the client error naturally up to the client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r771874749
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
Review comment:
Ah - right. I now remember something about that from your other PR...
I'll build off what you've done here and get this closed out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] holograph commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r413841973
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
+ guac_protocol_send_error(socket, message,
+ GUAC_PROTOCOL_STATUS_CLIENT_BAD_REQUEST);
Review comment:
Yes, and yes:
* The current guacd with the current mainstream (Java) client will simply error out due to socket read error;
* Once this is merged, the existing Java client will error out due to "unexpected opcode", which amounts to the same thing;
* Once the fix for GUACAMOLE-1048 is hopefully merged, the client will actually provide an honest-to-god error message :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a change in pull request #271: GUACAMOLE-1047: Notify connecting client on unrecognized connection ID
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #271:
URL: https://github.com/apache/guacamole-server/pull/271#discussion_r413834137
##########
File path: src/guacd/connection.c
##########
@@ -278,10 +278,18 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) {
proc = guacd_proc_map_retrieve(map, identifier);
new_process = 0;
- /* Warn if requested connection does not exist */
- if (proc == NULL)
- guacd_log(GUAC_LOG_INFO, "Connection \"%s\" does not exist.",
- identifier);
+ /* Warn and ward off client if requested connection does not exist */
+ if (proc == NULL) {
+ char message[2048];
+
+ snprintf(message, sizeof(message),
+ "Connection \"%s\" does not exist", identifier);
+
+ guacd_log(GUAC_LOG_INFO, message);
+ guac_protocol_send_error(socket, message,
+ GUAC_PROTOCOL_STATUS_CLIENT_BAD_REQUEST);
Review comment:
Ah, I see that you just put in a JIRA issue for the guacamole-common side :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org