necouchman commented on code in PR #627:
URL: https://github.com/apache/guacamole-server/pull/627#discussion_r2615333022
##########
src/libguac/guacamole/string.h:
##########
@@ -30,22 +30,24 @@
#include <string.h>
/**
- * Convert the provided unsigned integer into a string, returning the number of
- * characters written into the destination string, or a negative value if an
- * error occurs.
+ * Converts the given integer to a string safely. The resulting string will be
+ * written into the provided buffer, ensuring that the buffer is not exceeded.
+ * The conversion will fail if the buffer is too small to hold the result.
*
* @param dest
- * The destination string to copy the data into, which should already be
- * allocated and at a size that can handle the string representation of the
- * inteer.
+ * The buffer to write the converted string into.
+ *
+ * @param dest_size
+ * The size of the provided buffer, in bytes.
*
* @param integer
- * The unsigned integer to convert to a string.
- *
+ * The integer to convert to a string.
+ *
* @return
- * The number of characters written into the dest string.
+ * The number of characters written (excluding the null terminator), or
+ * -1 if the string was truncated, or a negative value if an error occurs.
Review Comment:
This might need slightly more clarification - `-1` is a negative value, so
is it `-2`, etc., if an error occurs?
##########
src/protocols/vnc/vnc.c:
##########
@@ -361,13 +361,12 @@ void* guac_vnc_client_thread(void* data) {
*/
if (settings->wol_wait_time > 0) {
guac_client_log(client, GUAC_LOG_DEBUG, "Sending Wake-on-LAN
packet, "
- "and pausing for %d seconds.", settings->wol_wait_time);
+ "and retrying connection check %d times every %d seconds.",
+ GUAC_WOL_DEFAULT_CONNECT_RETRIES, settings->wol_wait_time);
- /* char representation of a port should be, at most, 5 characters
plus terminator. */
- char* str_port = guac_mem_alloc(6);
- if (guac_itoa(str_port, settings->port) < 1) {
- guac_client_log(client, GUAC_LOG_ERROR, "Failed to convert
port to integer for WOL function.");
- guac_mem_free(str_port);
+ char str_port[GUAC_WOL_PORT_STRLEN];
Review Comment:
Is there a particular driver behind switching from dynamically allocating
this to statically allocating? I'm not necessarily opposed, just wondering what
the rationale is.
##########
src/protocols/ssh/settings.c:
##########
@@ -606,7 +607,6 @@ void guac_ssh_settings_free(guac_ssh_settings* settings) {
/* Free network connection information */
guac_mem_free(settings->hostname);
guac_mem_free(settings->host_key);
- guac_mem_free(settings->port);
Review Comment:
Why are we not freeing this anymore?
##########
src/protocols/rdp/settings.c:
##########
@@ -1109,8 +1110,8 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
/* Port for SFTP connection */
settings->sftp_port =
- guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
- IDX_SFTP_PORT, "22");
+ guac_user_parse_args_int_string_bound(user, GUAC_RDP_CLIENT_ARGS, argv,
+ IDX_SFTP_PORT, "22", 0, USHRT_MAX);
Review Comment:
Is `0` really a good valid minimum for SFTP? Is there any legitimate case in
which `0` would actually work? I've never tried it, so maybe it does - I'm
legitimately asking...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
