From bdb4f66a9d444e2c6d8be6d17cd64a98a13150c7 Mon Sep 17 00:00:00 2001 From: Ian Barwick Date: Thu, 31 Jan 2019 13:46:56 +0900 Subject: [PATCH] Add an Assert() to detect attempted array overflow in param_set...() functions Previously the code would do nothing if an attempt was made to add parameters if the array is already full. As the array is designed to contain all valid libpq connection parameters, there's no reason it should ever "overflow" like this. If there is, then it means the caller is attempting to add invalid values. Add an Assert() so we can easily detect this in the unlikely event it ever occurs. Noted after examining the issue raised in GitHub #533, which is nonsensical as it implies we'd be OK with writing beyond the end of the array, however it doesn't hurt to make it a bit clearer what is happening and why. --- dbutils.c | 48 ++++++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/dbutils.c b/dbutils.c index 12a99613..d5f465b6 100644 --- a/dbutils.c +++ b/dbutils.c @@ -519,6 +519,7 @@ param_set(t_conninfo_param_list *param_list, const char *param, const char *valu { int c; int value_len = strlen(value) + 1; + int param_len; /* * Scan array to see if the parameter is already set - if not, replace it @@ -538,24 +539,22 @@ param_set(t_conninfo_param_list *param_list, const char *param, const char *valu } /* - * Parameter not in array - add it and its associated value + * Sanity-check that the caller is not trying to overflow the array; + * in practice this is highly unlikely, and if it ever happens, this means + * something is highly wrong. */ - if (c < param_list->size) - { - int param_len = strlen(param) + 1; - - param_list->keywords[c] = pg_malloc0(param_len); - param_list->values[c] = pg_malloc0(value_len); - - strncpy(param_list->keywords[c], param, param_len); - strncpy(param_list->values[c], value, value_len); - } + Assert(c < param_list->size); /* - * It's theoretically possible a parameter couldn't be added as the array - * is full, but it's highly improbable so we won't handle it at the - * moment. + * Parameter not in array - add it and its associated value */ + param_len = strlen(param) + 1; + + param_list->keywords[c] = pg_malloc0(param_len); + param_list->values[c] = pg_malloc0(value_len); + + strncpy(param_list->keywords[c], param, param_len); + strncpy(param_list->values[c], value, value_len); } @@ -567,6 +566,7 @@ param_set_ine(t_conninfo_param_list *param_list, const char *param, const char * { int c; int value_len = strlen(value) + 1; + int param_len; /* * Scan array to see if the parameter is already set - if so, do nothing @@ -580,19 +580,23 @@ param_set_ine(t_conninfo_param_list *param_list, const char *param, const char * } } + /* + * Sanity-check that the caller is not trying to overflow the array; + * in practice this is highly unlikely, and if it ever happens, this means + * something is highly wrong. + */ + Assert(c < param_list->size); + /* * Parameter not in array - add it and its associated value */ - if (c < param_list->size) - { - int param_len = strlen(param) + 1; + param_len = strlen(param) + 1; - param_list->keywords[c] = pg_malloc0(param_len); - param_list->values[c] = pg_malloc0(value_len); + param_list->keywords[c] = pg_malloc0(param_len); + param_list->values[c] = pg_malloc0(value_len); - strncpy(param_list->keywords[c], param, param_len); - strncpy(param_list->values[c], value, value_len); - } + strncpy(param_list->keywords[c], param, param_len); + strncpy(param_list->values[c], value, value_len); }