mike-jumper commented on a change in pull request #228: GUACAMOLE-221: Implement guacd support for prompting URL: https://github.com/apache/guacamole-server/pull/228#discussion_r371009915
########## File path: src/protocols/rdp/argv.c ########## @@ -0,0 +1,192 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "argv.h" +#include "rdp.h" + +#include <guacamole/protocol.h> +#include <guacamole/socket.h> +#include <guacamole/user.h> + +#include <pthread.h> +#include <stdlib.h> +#include <string.h> + +/** + * All RDP connection settings which may be updated by unprivileged users + * through "argv" streams. + */ +typedef enum guac_rdp_argv_setting { + + /** + * The username of the connection. + */ + GUAC_RDP_ARGV_SETTING_USERNAME, + + /** + * The password to authenticate the connection. + */ + GUAC_RDP_ARGV_SETTING_PASSWORD, + + /** + * The domain to use for connection authentication. + */ + GUAC_RDP_ARGV_SETTING_DOMAIN, + +} guac_rdp_argv_setting; + +/** + * The value or current status of a connection parameter received over an + * "argv" stream. + */ +typedef struct guac_rdp_argv { + + /** + * The specific setting being updated. + */ + guac_rdp_argv_setting setting; + + /** + * Buffer space for containing the received argument value. + */ + char buffer[GUAC_RDP_ARGV_MAX_LENGTH]; + + /** + * The number of bytes received so far. + */ + int length; + +} guac_rdp_argv; + +/** + * Handler for "blob" instructions which appends the data from received blobs + * to the end of the in-progress argument value buffer. + * + * @see guac_user_blob_handler + */ +static int guac_rdp_argv_blob_handler(guac_user* user, + guac_stream* stream, void* data, int length) { + + guac_rdp_argv* argv = (guac_rdp_argv*) stream->data; + + /* Calculate buffer size remaining, including space for null terminator, + * adjusting received length accordingly */ + int remaining = sizeof(argv->buffer) - argv->length - 1; + if (length > remaining) + length = remaining; + + /* Append received data to end of buffer */ + memcpy(argv->buffer + argv->length, data, length); + argv->length += length; + + return 0; + +} + +/** + * Handler for "end" instructions which applies the changes specified by the + * argument value buffer associated with the stream. + * + * @see guac_user_end_handler + */ +static int guac_rdp_argv_end_handler(guac_user* user, + guac_stream* stream) { + + guac_client* client = user->client; + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + guac_rdp_settings* settings = rdp_client->settings; + + /* Append null terminator to value */ + guac_rdp_argv* argv = (guac_rdp_argv*) stream->data; + argv->buffer[argv->length] = '\0'; Review comment: Yep, definitely necessary. Nothing handling the received data as a string will be able to function safely without that null terminator. The beauty of `strndup(foo, length)` is that it will automatically add that null terminator and ensure that there is space for it within the newly-allocated buffer. Assuming the string within the buffer is indeed `length` characters long, `strndup(..., length)` would allocate buffer space of at least `length + 1` bytes. Overall: * We do need that null terminator. * It feels weird to know the length of the string ahead of time, add a null terminator, and then recalculate that length value with `strlen()` based on the null terminator we just added. * If we're going to use `strlen()` to feed a `malloc()` and `strcpy()`, that's a common enough operation that there's a standard POSIX function for that: `strdup()` * `strdup()` depends on having a null terminator within the string being copied, yet it's unsafe to add a null terminator within that buffer because we don't know for certain there is space. Our choices then are: 1. Use `strndup()` OR 2. Use `malloc(length + 1)`, `memcpy(..., length)` the old buffer to the new buffer, and then add the null terminator to the new buffer. ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
