GUACAMOLE-269: Allow backspace key behavior to be configured #156

necouchman · 2018-02-26T19:43:52Z

Server-side changes that allow the backspace key to be configured, and, for the SSH protocol, also pass through that behavior to the SSH server via the TTY Mode Encoding options. This replaces #153.

mike-jumper · 2018-02-27T06:21:22Z

src/terminal/terminal.c

@@ -1594,7 +1599,7 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
        /* Non-printable keys */
        else {

-            if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+            if (keysym == 0xFF08) return guac_terminal_send_string(term, &term->backspace); /* Backspace */


guac_terminal_send_string() requires a null-terminated string. As &term->backspace is a pointer to a single character, there is no guarantee of null terminator, and this may read without bound, resulting in connection closure or dumping of memory data to the remote terminal.

mike-jumper · 2018-02-27T06:22:55Z

src/protocols/ssh/settings.h

@@ -223,6 +223,11 @@ typedef struct guac_ssh_settings {
     */
    int server_alive_interval;

+    /**
+     * The decismal ASCII code of the command to send for backspace.


"decismal" is a typo, but that may be moot as this is technically incorrect. While the string from which this int was parsed may have been decimal, at this point it's just an int.

mike-jumper · 2018-02-27T06:25:50Z

src/protocols/ssh/ttymode.c

+#include <string.h>
+
+guac_ssh_ttymodes init_ttymodes() {
+    // Simple allocation for a placeholder


Comments for C source within Guacamole should use /* ... */ syntax, even if only a single line.

mike-jumper · 2018-02-27T06:29:13Z

src/protocols/ssh/ttymode.c

+        0
+    };
+
+    return empty_modes;


Assigning/returning a struct by value like this actually does an implicit copy in C.

Updated this to a pointer - not sure if that's what you had in mind, but I think it works better any way with what I was trying to do in the other functions.

mike-jumper · 2018-02-27T06:34:44Z

Before I continue with this review, do you think that init_ttymodes() / add_ttymode() / sizeof_ttymodes() / ttymodes_to_array() might be overkill for the problem at hand? There's a lot of complex logic and dynamic allocation happening there for what boils down to a single opcode.

necouchman · 2018-02-27T14:18:42Z

Before I continue with this review, do you think that init_ttymodes() / add_ttymode() / sizeof_ttymodes() / ttymodes_to_array() might be overkill for the problem at hand? There's a lot of complex logic and dynamic allocation happening there for what boils down to a single opcode.

You're probably right - it definitely felt a little overkill-ish while writing it; however, I was trying to both solve the problem of reading the new SSH setting into the array while also making it so that this could be expanded and reused easily in the future. While this particular JIRA issue deals with the behavior of the backspace key, there are several other TTY Mode Encoding options specified in the SSH RFC, and I didn't want to make the assumption that this is the only one we'd ever be interested in implementing.

However, if you think that's a problem we should worry about when the second opcode comes along, that's fine with me - I can back off this and just write a function that generates the pointer array with the VERASE opcode and value and the ending opcode. Let me know which route you think is best and I'll be happy to do it.

mike-jumper · 2018-03-03T00:38:49Z

You're probably right - it definitely felt a little overkill-ish while writing it; however, I was trying to both solve the problem of reading the new SSH setting into the array while also making it so that this could be expanded and reused easily in the future. ...

Future-proofing these changes is not a bad idea, but let's do so without complex dynamic allocation. In this case:

We know ahead of time the necessary size of the mode array.
The mode array itself needs to live only for the duration of that initial call which allocates the PTY.

For the sake of simplicity, I'd say we should really just use a statically-allocated array for now. If you feel that dynamic allocation would be more appropriate, then we should make use of our knowledge of the necessary size and allocate the necessary space ahead of time (without resizing) and be careful to free the allocated data once it is no longer needed.

necouchman · 2018-03-04T02:28:34Z

@mike-jumper
Well, I stuck with dynamic allocation, mainly because I was having a silly amount of trouble figuring out how to properly handle standard arrays in the data structure. But, I'm allocating everything ahead of time with known sizes, so hopefully that addresses any concerns. I'm happy to take other suggestions, though.

I also added some calls to free the data structures - I think I got everything, but worth double-checking to make sure.

mike-jumper · 2018-03-06T20:54:35Z

Hm ... there has to be a cleaner way. The current implementation is:

An internal representation of the TTY modes array is dynamically allocated with guac_ssh_ttymodes_init()
The single TTY mode is added to that array dynamically with guac_ssh_ttymodes_add()
That internal representation is converted to the representation required by SSH through more dynamic allocation via guac_ssh_ttymodes_size() and guac_ssh_ttymodes_to_array()
All memory allocated above is manually freed via calls to free().

I have a few problems with this. We are dynamically allocating an intermediate representation which really has no purpose other than to be converted into another dynamically-allocated representation, both of which have the same lifespan. Abstraction is a great thing, but the complexity of this abstraction seems to outweigh the intended benefit.

I suggest:

Look for ways to avoid so much dynamic allocation. Perhaps there is a way to avoid the intermediate representation which ultimately gets thrown away?
Look for ways to abstract things such that they (1) simplify your current implementation and (2) make future development easier. Perhaps the intermediate representation could be passed in directly without dynamic allocation? Might variadic functions achieve this better? Something like:
```
size = guac_ssh_ttymodes_init(&some_static_buffer, sizeof(some_static_buffer),
            GUAC_SSH_TTY_OP_VERASE, settings->backspace);
```
would abstract things away, require only a single function call, and would not depend on dynamic allocation.

necouchman · 2018-03-06T21:13:09Z

Hm ... there has to be a cleaner way.

Oh, I have no doubt at all there is a cleaner way! :-)

Perhaps there is a way to avoid the intermediate representation which ultimately gets thrown away?

Yeah, I'll see what I can do about this. I was initially trying to create a data structure that could be cast into a char * and passed directly to the libssh2_channel_request_pty_ex() function. I thought I remembered from somewhere in my past that doing this:

struct ttymode {
    char opcode;
    uint32_t value;
} ttymode;

Would result in memory being allocated in such a way that it could then be cast into char * and you'd end up with the single opcode byte and the four value bytes all together, which is what the function expects. Doesn't seem to work out, though. If that is supposed to work, that seems like the simplest route to be, because the members can then be accessed easily (array[0].opcode and array[0].value) but then the entire thing can be passed directly to the function. Am I remembering that correctly, or is there something else I'm not factoring in to how that allocation works?

mike-jumper · 2018-03-06T21:27:21Z

Unfortunately, that isn't safe across different architectures.

A C struct provides a guarantee regarding order in memory (opcode will definitely come before value), but it does not provide guarantees regarding alignment (there may be padding before or after those members). Some systems may have alignment requirements that mean opcode will not be directly adjacent to value in memory.

There are also no guarantees regarding byte order for things like a uint32_t. Mathematical operations will work transparently, but the byte order used by the architecture for 32-bit integers may differ from the byte order required by the SSH server.

…a structures.

necouchman · 2018-03-08T16:24:48Z

Okay, I've removed all of the dynamic allocation and most of the functions, aside from init. I'm using a variadic function, with a custom data structure to simplify counting and passing arguments through to that function. Let me know how this looks.

As usual, Mike, thanks for the patience and help working through this...

…ion.

necouchman · 2018-03-17T01:48:51Z

@mike-jumper Any other changes needed to wrap this one up?

mike-jumper · 2018-03-18T06:44:34Z

src/protocols/ssh/ttymode.h

+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and


Is there a way you could describe this which provides information/context beyond the members of the structure?

Took a stab at some more thorough documentation.

mike-jumper · 2018-03-18T06:44:45Z

src/protocols/ssh/ttymode.h

+ */
+typedef struct guac_ssh_ttymode {
+    char opcode;
+    uint32_t value;


Structure members need to be documented.

Documented.

mike-jumper · 2018-03-18T06:46:34Z

src/protocols/ssh/ttymode.h

+} guac_ssh_ttymode;
+
+/**
+ * Initialize an array with the specified opcode/value


Initializes an array ... why? What's special about the array? Anything you can think of that might make the use of this function clear?

Beefed up this documentation.

mike-jumper · 2018-03-18T06:51:43Z

src/terminal/terminal.c

-            if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+            /* Backspace can vary based on configuration of terminal by client. */
+            if (keysym == 0xFF08) {
+                char* backspace_str = malloc(sizeof(char) * 2);


Keep in mind that dynamic allocation incurs overhead. In some cases, it makes good sense to do this, but it's hard to justify for a 2-byte string. A static buffer would be more appropriate here, particularly since you can initialize things cleanly and clearly inline.

Switched to static.

mike-jumper · 2018-03-18T06:52:29Z

src/terminal/terminal.c

+                char* backspace_str = malloc(sizeof(char) * 2);
+                backspace_str[0] = term->backspace;
+                backspace_str[1] = '\0';
+                return guac_terminal_send_string(term, backspace_str);


Returning here without freeing the memory allocated above will leak memory each time backspace is pressed. Probably shouldn't be dynamically allocating at all, though (see above).

Staticized.

mike-jumper · 2018-03-18T06:56:47Z

src/protocols/ssh/ttymode.c

+    /* Put the end opcode in the last opcode space */
+    opcode_array[num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE] = GUAC_SSH_TTY_OP_END;
+
+    return 0;


The documentation states that this function returns the size of the array (or rather, I assume, the number of bytes written to the array), but this isn't happening here.

Documentation has been fixed.

mike-jumper · 2018-03-18T06:57:59Z

src/protocols/ssh/ttymode.h

+
+/**
+ * Initialize an array with the specified opcode/value
+ * pairs, and return the size, in bytes, of the array.


Do mean the number of bytes written to the array? Ignoring that the size of the array is already known to the caller, returning the size of the array is not particularly useful, especially since the array may be larger than necessary.

Documentation updated.

mike-jumper · 2018-03-18T07:04:34Z

src/protocols/ssh/ttymode.c

+        int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+        /* Get the next argument to this function */
+        guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);


The structure in this case is rather small, so this does not make much of a difference in this case, but beware that passing structures by value will result in a full copy of that structure, with this assignment resulting in another full copy.

Is it possible to do this by pointer/reference rather than value? I'm not familiar with the va_arg macro and how to use it - do I just change the argument to &guac_ssh_ttymode and the assignment to guac_ssh_ttymode* ttymode?

I think I got this going, too - again, probably worth someone taking a look just to make sure. Seems to work, though.

mike-jumper · 2018-03-18T07:05:56Z

src/protocols/ssh/ttymode.c

+
+        /* Place opcode and value in array */
+        opcode_array[offset] = ttymode.opcode;
+        opcode_array[offset + 1] = (ttymode.value >> 24) & 0xFF;


You don't need to manually calculate the offset for each byte written. If you have a pointer to the next byte to be written within the array, this could be as simple as:

*(current++) = whatever_you_want_to_write;

Okay, took a stab at this, and I think I got it working correctly - at least, the code runs and behaves as expected. Might be worth someone looking it over, though, just to make sure I did it in a way that isn't going to hit some corner case at some point down the road.

mike-jumper · 2018-03-18T07:07:09Z

src/protocols/ssh/ttymode.c

+    }
+
+    /* Put the end opcode in the last opcode space */
+    opcode_array[num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE] = GUAC_SSH_TTY_OP_END;


To confirm: the final "END" does not need the full 5-byte opcode and value? Just the opcode?

That's correct - as soon as it encounters the 0 opcode in one of the expected positions (5 byte offsets, basically) it stops.

necouchman · 2018-03-24T20:00:01Z

Okay, @mike-jumper, next round of updates done - let me know what else needs attention!

mike-jumper · 2018-04-02T03:23:53Z

src/protocols/ssh/ttymode.c

+    /* Put the end opcode in the last opcode space */
+    *(current) = GUAC_SSH_TTY_OP_END;
+
+    return 0;


I recommend instead returning the size of the data written to the array, with zero indicating failure. This would remove the need for the caller to know ahead of time the size of the data this function will write (which is an odd requirement for what is otherwise an abstraction).

Sounds good - is there a more elegant way than just adding an integer byte counter and incrementing along the way?

I've gone the byte counter route - if you have a suggestion for another way to do it, let me know.

mike-jumper · 2018-04-02T03:28:57Z

src/terminal/terminal.c

+            if (keysym == 0xFF08) {
+                char backspace_str[2];
+                backspace_str[0] = term->backspace;
+                backspace_str[1] = '\0';


You can clean this up as:

char backspace_str[2] = { term->backspace, '\0' };

or even:

char backspace_str[] = { term->backspace, '\0' };

Of course. Done & thanks.

mike-jumper · 2018-04-02T03:30:54Z

src/protocols/ssh/ssh.c

+    /* Set up the ttymode array prior to requesting the PTY */
+    if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes),
+            num_tty_opcodes, (guac_ssh_ttymode){ GUAC_SSH_TTY_OP_VERASE, settings->backspace}))
+        guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error configuring TTY mode encoding.");


What do you mean by "configuring TTY mode encoding"? Is there a way to phrase this which would better represent what is failing?

Tried to clean up this error message bit.

mike-jumper · 2018-04-02T03:34:47Z

src/protocols/ssh/ttymode.c

+
+    /* Initialize the variable argument list. */
+    va_list args;
+    va_start(args, num_opcodes);


There must be a matching call to va_end() before the end of this function.

mike-jumper · 2018-04-02T03:41:47Z

src/protocols/ssh/ssh.c

@@ -296,9 +301,15 @@ void* ssh_client_thread(void* data) {

    }

+    /* Set up the ttymode array prior to requesting the PTY */
+    if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes),
+            num_tty_opcodes, (guac_ssh_ttymode){ GUAC_SSH_TTY_OP_VERASE, settings->backspace}))


Looking at the source for guac_ssh_ttymodes_init(), doesn't this need to be a pointer to a guac_ssh_ttymode? ie: &((guac_ssh_ttymode) { GUAC_SSH_TTY_OP_VERASE, settings->backspace }) ?

Alternatively, there's no need for all these parameters to be of the same type. It would also be possible to do something like:

if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes), GUAC_SSH_TTY_OP_VERASE, settings->backspace, GUAC_SSH_TTY_OP_END))

The code is your oyster.

Refactored the guac_ssh_ttymodes_init() function. I may have introduced some other bugs or conditions that need to be handled, so let me know what you think about it.

mike-jumper · 2018-04-02T03:47:33Z

src/protocols/ssh/ssh.c

@@ -191,6 +192,10 @@ void* ssh_client_thread(void* data) {
        return NULL;
    }

+    /* Initialize a ttymode array */
+    const int num_tty_opcodes = 1;
+    char ssh_ttymodes[(GUAC_SSH_TTY_OPCODE_SIZE * num_tty_opcodes) + 1];


Not really a dealbreaker, but if you want to ensure this is a constant expression while also abstracting away the calculations required, this could be handled via a macro defined in ttymode.h. For example:

#define GUAC_SSH_TTYMODES_SIZE(num_opcodes) ((GUAC_SSH_TTY_OPCODE_SIZE * num_opcodes) + 1)

You could then rely upon:

char ssh_ttymodes[GUAC_SSH_TTYMODES_SIZE(1)];

... and, of course, documenting within guac_ssh_ttymodes_init() how the minimum size for the provided buffer should be calculated.

Magic.

Brilliant. I've used your macro and refactored code and comments with that.

mike-jumper · 2018-04-02T03:49:11Z

src/protocols/ssh/ssh.c

@@ -191,6 +192,10 @@ void* ssh_client_thread(void* data) {
        return NULL;
    }

+    /* Initialize a ttymode array */


Point of clarification - nothing is being initialized here, only declared. With that much being obvious (that the array is being declared), rather than change "Initialize" to "Declare", I'd say just kill this line.

necouchman · 2018-04-02T13:24:02Z

Cleaned those items up.

…update comments.

mike-jumper · 2018-04-02T18:35:33Z

src/protocols/ssh/ssh.c

+    int ttymodeBytes = guac_ssh_ttymodes_init(ssh_ttymodes,
+            GUAC_SSH_TTY_OP_VERASE, settings->backspace, GUAC_SSH_TTY_OP_END);
+    if (ttymodeBytes < 1)
+        guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error storing TTY mode encoding \


This will actually log:

Error storing TTY mode encoding opcodes and values in array.

as the whitespace at the beginning of the following line is part of the string.

To split strings in C, all you need to do is place two strings next to each other. They're automatically concatenated by the compiler:

guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error storing TTY mode encoding " "opcodes and values in array.");

As this message is meant for the user/admin, I suggest rephrasing to describe things from that perspective. For example:

Unable to set TTY modes. Backspace may not work as expected.

at the warning level may make more sense.

Changed to warning.

mike-jumper · 2018-04-02T18:36:56Z

src/protocols/ssh/ttymode.c

+    /* We're done processing arguments. */
+    va_end(args);
+
+    return bytes;


FYI - you can actually do:

current - opcode_array

Yes, of course.

mike-jumper · 2018-04-02T18:37:53Z

src/protocols/ssh/ttymode.h

+ * the size of the number of opcodes plus the single byte
+ * end opcode.
+ */
+#define GUAC_SSH_TTYMODES_SIZE(num_opcodes) ((GUAC_SSH_TTY_OPCODE_SIZE * num_opcodes) + 1)


Please document num_opcodes with @param, just as you would for a function.

Documented that, and also documented the return value with @returns - not sure if that should be done, too?

Yup, definitely. Good catch.

necouchman · 2018-04-02T19:07:09Z

Next round done.

necouchman mentioned this pull request Feb 26, 2018

GUACAMOLE-269: Add basic support for sending TTY mode encoding over SSH #153

Closed

mike-jumper reviewed Feb 27, 2018

View reviewed changes

necouchman added 12 commits March 8, 2018 10:48

GUACAMOLE-269: Add basic support for sending TTY mode encoding.

f1bf70a

GUACAMOLE-269: Move constant declaration to ttymode.c

5583748

GUACAMOLE-269: Add documentation for the defines and variables.

2ace938

GUACAMOLE-269: Allow backspace key to be configured.

46e908c

GUACAMOLE-269: Use backspace config to set up tty modes.

fd58d31

GUACAMOLE-269: Fix up style in comments.

9bd2832

GUACAMOLE-269: Name functions per Guacamole standards.

c286668

GUACAMOLE-269: Backspace key should send null-terminated string.

dd78d23

GUACAMOLE-269: Remove debug code.

33cca46

GUACAMOLE-269: Change struct to struct pointer.

64ca77f

GUACAMOLE-269: Fix minor style issues and update comments.

c3e1b2a

GUACAMOLE-269: Get rid of dynamic allocation and properly free up dat…

dd7522b

…a structures.

necouchman force-pushed the working/backspace-config branch 2 times, most recently from ae1664d to 40e3508 Compare March 8, 2018 16:23

necouchman force-pushed the working/backspace-config branch from 40e3508 to 848bd1e Compare March 8, 2018 16:32

GUACAMOLE-269: Remove all dynamic allocation and simplify implementat…

45b832b

…ion.

necouchman force-pushed the working/backspace-config branch from 848bd1e to 45b832b Compare March 8, 2018 16:36

mike-jumper requested changes Mar 18, 2018

View reviewed changes

GUACAMOLE-269: Comment and spelling updates.

86dde85

necouchman added 3 commits March 24, 2018 14:54

GUACAMOLE-269: Remove unnecessary dynamic allocation.

112ce52

GUACAMOLE-269: More documentation updates.

11136f7

GAUCAMOLE-269: Memory effeciency updates.

e16bfd7

mike-jumper requested changes Apr 2, 2018

View reviewed changes

necouchman added 2 commits April 2, 2018 07:47

GUACAMOLE-269: Clean up terminal backspace initialization.

c898f35

GUACAMOLE-269: Changes to initializing opcode array.

ea946f2

GUACAMOLE-269: Remove unnecessary data structure and array size, and …

b441181

…update comments.

necouchman force-pushed the working/backspace-config branch from a91dfe2 to b441181 Compare April 2, 2018 14:44

mike-jumper requested changes Apr 2, 2018

View reviewed changes

necouchman added 2 commits April 2, 2018 15:04

GUACAMOLE-269: Clean up logging and comments, and simplify code.

7453bc8

GUACAMOLE-269: Don't abort on ttymode issue, just log a warning.

dc1918b

mike-jumper approved these changes Apr 2, 2018

View reviewed changes

asfgit merged commit dc1918b into apache:master Apr 2, 2018

necouchman deleted the working/backspace-config branch April 2, 2018 20:48

GUACAMOLE-269: Allow backspace key behavior to be configured #156

GUACAMOLE-269: Allow backspace key behavior to be configured #156

Conversation

necouchman commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necouchman Feb 27, 2018 • edited

Choose a reason for hiding this comment

mike-jumper commented Feb 27, 2018

necouchman commented Feb 27, 2018

mike-jumper commented Mar 3, 2018

necouchman commented Mar 4, 2018

mike-jumper commented Mar 6, 2018

necouchman commented Mar 6, 2018

mike-jumper commented Mar 6, 2018

necouchman commented Mar 8, 2018

necouchman commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necouchman commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necouchman commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necouchman commented Apr 2, 2018

necouchman Feb 27, 2018 •

edited