From 7bce1bd3723cc8fa1405f997de9796ffd06c04df Mon Sep 17 00:00:00 2001 From: John Gemignani Date: Wed, 31 Jul 2024 11:37:16 -0700 Subject: [PATCH] Fix issue 1986: Failure creating label name close to MAX_LABEL_NAME_LEN (#1995) Fixed the issue with creating a label name close to MAX_LABEL_NAME_LEN. Well, sort of. The issue here is that a label name is the name of a relation in PostgreSQL. While the relation name is a char * it is basically converted to a PG type Name when it is used to create a relation. This conversion is a silent truncation, there are no warnings or error messages. The Name type is defined as follows - /* * Representation of a Name: effectively just a C string, but null-padded to * exactly NAMEDATALEN bytes. The use of a struct is historical. */ typedef struct nameData { char data[NAMEDATALEN]; } NameData; typedef NameData *Name; This effectively gives us a max label name length of NAMEDATALEN - 1. /* * Maximum length for identifiers (e.g. table names, column names, * function names). Names actually are limited to one fewer byte than this, * because the length must include a trailing zero byte. * * This should be at least as much as NAMEDATALEN of the database the * applications run against. */ #define NAMEDATALEN 64 Since there isn't a way to get around this, the code was modified to reflect the usage of NAMEDATALEN for the length of label names. This required modifying the input parameters to be cstrings, for the functions `create_vlabel` and `create_elabel` which allows us to tell when NAMEDATALEN has been exceeded. Additionally, the function `is_valid_label` was renamed to `is_valid_label_name` for consistency. Regression tests were updated and added. --- age--1.5.0--y.y.y.sql | 17 +++++ regress/expected/name_validation.out | 54 ++++++++++++++- regress/sql/name_validation.sql | 22 ++++++ sql/age_main.sql | 4 +- src/backend/commands/label_commands.c | 98 ++++++++++++++------------- src/backend/utils/name_validation.c | 7 +- src/include/utils/name_validation.h | 4 +- 7 files changed, 152 insertions(+), 54 deletions(-) diff --git a/age--1.5.0--y.y.y.sql b/age--1.5.0--y.y.y.sql index 1620dfb4f..710a196a8 100644 --- a/age--1.5.0--y.y.y.sql +++ b/age--1.5.0--y.y.y.sql @@ -129,3 +129,20 @@ CREATE FUNCTION ag_catalog.graph_exists(graph_name name) RETURNS agtype LANGUAGE c AS 'MODULE_PATHNAME', 'age_graph_exists'; + +CREATE FUNCTION ag_catalog.age_is_valid_label_name(agtype) + RETURNS boolean + LANGUAGE c + IMMUTABLE +PARALLEL SAFE +AS 'MODULE_PATHNAME'; + +CREATE OR REPLACE FUNCTION ag_catalog.create_vlabel(graph_name cstring, label_name cstring) + RETURNS void + LANGUAGE c + AS 'MODULE_PATHNAME'; + +CREATE OR REPLACE FUNCTION ag_catalog.create_elabel(graph_name cstring, label_name cstring) + RETURNS void + LANGUAGE c + AS 'MODULE_PATHNAME'; diff --git a/regress/expected/name_validation.out b/regress/expected/name_validation.out index 232582bc2..a9f804076 100644 --- a/regress/expected/name_validation.out +++ b/regress/expected/name_validation.out @@ -233,8 +233,10 @@ NOTICE: graph "graph123" has been created -- length -- invalid SELECT create_vlabel('graph123', ''); +WARNING: label name length not in range (1 <= length <= 63) length = 0 ERROR: label name is invalid SELECT create_elabel('graph123', ''); +WARNING: label name length not in range (1 <= length <= 63) length = 0 ERROR: label name is invalid -- valid SELECT create_vlabel('graph123', 'labelx'); @@ -396,9 +398,57 @@ SELECT * from cypher('graph123', $$ return is_valid_label_name('label2') $$) as true (1 row) +-- issue 1986: label name validation of long names. +-- Label names are relation names which are restricted to NAMEDATALEN-1 in size. +-- However, we can't validate PG type Names due to namein() truncating anything +-- over NAMEDATALEN-1. To allow the label names to be checked over NAMEDATELEN-1 +-- we changed the input type from PG's Name to cstring. These checks are to +-- verify that these can now be caught. +-- +-- should return false and a warning. +SELECT * from cypher('graph123', $$ return is_valid_label_name('label01234567890123456789012345678901234567890123456789012345678') $$) as (result agtype); +WARNING: label name length not in range (1 <= length <= 63) length = 64 + result +-------- + false +(1 row) + +-- should be successful +SELECT * from cypher('graph123', $$ return is_valid_label_name('label0123456789012345678901234567890123456789012345678901234567') $$) as (result agtype); + result +-------- + true +(1 row) + +-- +-- now check vlabel creation, should fail +SELECT create_vlabel('graph123', 'vlabel01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678'); +WARNING: label name length not in range (1 <= length <= 63) length = 95 +ERROR: label name is invalid +-- should be successful +SELECT create_vlabel('graph123', 'vlabel012345678901234567890123456789012345678901234567890123456'); +NOTICE: VLabel "vlabel012345678901234567890123456789012345678901234567890123456" has been created + create_vlabel +--------------- + +(1 row) + +-- +-- now check elabel creation, should fail +SELECT create_elabel('graph123', 'elabel0123456789012345678901234567890123456789012345678901234567'); +WARNING: label name length not in range (1 <= length <= 63) length = 64 +ERROR: label name is invalid +-- should be okay +SELECT create_elabel('graph123', 'elabel012345678901234567890123456789012345678901234567890123456'); +NOTICE: ELabel "elabel012345678901234567890123456789012345678901234567890123456" has been created + create_elabel +--------------- + +(1 row) + -- clean up SELECT drop_graph('graph123', true); -NOTICE: drop cascades to 18 other objects +NOTICE: drop cascades to 20 other objects DETAIL: drop cascades to table graph123._ag_label_vertex drop cascades to table graph123._ag_label_edge drop cascades to table graph123.labelx @@ -417,6 +467,8 @@ drop cascades to table graph123.mylabel drop cascades to table graph123."A" drop cascades to table graph123.mylabel2 drop cascades to table graph123."C" +drop cascades to table graph123.vlabel012345678901234567890123456789012345678901234567890123456 +drop cascades to table graph123.elabel012345678901234567890123456789012345678901234567890123456 NOTICE: graph "graph123" has been dropped drop_graph ------------ diff --git a/regress/sql/name_validation.sql b/regress/sql/name_validation.sql index bfb5d886b..0c96a25f2 100644 --- a/regress/sql/name_validation.sql +++ b/regress/sql/name_validation.sql @@ -153,6 +153,28 @@ SELECT * from cypher('graph123', $$ return is_valid_label_name('2label') $$) as SELECT * from cypher('graph123', $$ return is_valid_label_name('label1') $$) as (result agtype); SELECT * from cypher('graph123', $$ return is_valid_label_name('label2') $$) as (result agtype); +-- issue 1986: label name validation of long names. +-- Label names are relation names which are restricted to NAMEDATALEN-1 in size. +-- However, we can't validate PG type Names due to namein() truncating anything +-- over NAMEDATALEN-1. To allow the label names to be checked over NAMEDATELEN-1 +-- we changed the input type from PG's Name to cstring. These checks are to +-- verify that these can now be caught. +-- +-- should return false and a warning. +SELECT * from cypher('graph123', $$ return is_valid_label_name('label01234567890123456789012345678901234567890123456789012345678') $$) as (result agtype); +-- should be successful +SELECT * from cypher('graph123', $$ return is_valid_label_name('label0123456789012345678901234567890123456789012345678901234567') $$) as (result agtype); +-- +-- now check vlabel creation, should fail +SELECT create_vlabel('graph123', 'vlabel01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678'); +-- should be successful +SELECT create_vlabel('graph123', 'vlabel012345678901234567890123456789012345678901234567890123456'); +-- +-- now check elabel creation, should fail +SELECT create_elabel('graph123', 'elabel0123456789012345678901234567890123456789012345678901234567'); +-- should be okay +SELECT create_elabel('graph123', 'elabel012345678901234567890123456789012345678901234567890123456'); + -- clean up SELECT drop_graph('graph123', true); diff --git a/sql/age_main.sql b/sql/age_main.sql index 3bea3daca..59ada0f9f 100644 --- a/sql/age_main.sql +++ b/sql/age_main.sql @@ -100,12 +100,12 @@ CREATE FUNCTION ag_catalog.drop_graph(graph_name name, cascade boolean = false) LANGUAGE c AS 'MODULE_PATHNAME'; -CREATE FUNCTION ag_catalog.create_vlabel(graph_name name, label_name name) +CREATE FUNCTION ag_catalog.create_vlabel(graph_name cstring, label_name cstring) RETURNS void LANGUAGE c AS 'MODULE_PATHNAME'; -CREATE FUNCTION ag_catalog.create_elabel(graph_name name, label_name name) +CREATE FUNCTION ag_catalog.create_elabel(graph_name cstring, label_name cstring) RETURNS void LANGUAGE c AS 'MODULE_PATHNAME'; diff --git a/src/backend/commands/label_commands.c b/src/backend/commands/label_commands.c index c2dc9681b..05c4b8495 100644 --- a/src/backend/commands/label_commands.c +++ b/src/backend/commands/label_commands.c @@ -130,7 +130,7 @@ Datum age_is_valid_label_name(PG_FUNCTION_ARGS) label_name = pnstrdup(agtv_value->val.string.val, agtv_value->val.string.len); - is_valid = is_valid_label(label_name, 0); + is_valid = is_valid_label_name(label_name, 0); pfree(label_name); if (is_valid) @@ -155,17 +155,11 @@ PG_FUNCTION_INFO_V1(create_vlabel); Datum create_vlabel(PG_FUNCTION_ARGS) { - char *graph; - Name graph_name; - char *graph_name_str; + char *graph_name; Oid graph_oid; List *parent; - RangeVar *rv; - - char *label; - Name label_name; - char *label_name_str; + char *label_name; /* checking if user has not provided the graph name */ if (PG_ARGISNULL(0)) @@ -181,42 +175,49 @@ Datum create_vlabel(PG_FUNCTION_ARGS) errmsg("label name must not be NULL"))); } - graph_name = PG_GETARG_NAME(0); - label_name = PG_GETARG_NAME(1); + graph_name = PG_GETARG_CSTRING(0); + label_name = PG_GETARG_CSTRING(1); - graph_name_str = NameStr(*graph_name); - label_name_str = NameStr(*label_name); + /* validate the graph and label names */ + if (is_valid_graph_name(graph_name) == 0) + { + ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("graph name is invalid"))); + } + + if (is_valid_label_name(label_name, 0) == 0) + { + ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("label name is invalid"))); + } /* Check if graph does not exist */ - if (!graph_exists(graph_name_str)) + if (!graph_exists(graph_name)) { ereport(ERROR, (errcode(ERRCODE_UNDEFINED_SCHEMA), - errmsg("graph \"%s\" does not exist.", graph_name_str))); + errmsg("graph \"%s\" does not exist.", graph_name))); } - graph_oid = get_graph_oid(graph_name_str); + graph_oid = get_graph_oid(graph_name); /* Check if label with the input name already exists */ - if (label_exists(label_name_str, graph_oid)) + if (label_exists(label_name, graph_oid)) { ereport(ERROR, (errcode(ERRCODE_UNDEFINED_SCHEMA), - errmsg("label \"%s\" already exists", label_name_str))); + errmsg("label \"%s\" already exists", label_name))); } /* Create the default label tables */ - graph = graph_name->data; - label = label_name->data; - - rv = get_label_range_var(graph, graph_oid, AG_DEFAULT_LABEL_VERTEX); + rv = get_label_range_var(graph_name, graph_oid, AG_DEFAULT_LABEL_VERTEX); parent = list_make1(rv); - create_label(graph, label, LABEL_TYPE_VERTEX, parent); + create_label(graph_name, label_name, LABEL_TYPE_VERTEX, parent); ereport(NOTICE, - (errmsg("VLabel \"%s\" has been created", NameStr(*label_name)))); + (errmsg("VLabel \"%s\" has been created", label_name))); PG_RETURN_VOID(); } @@ -235,17 +236,11 @@ PG_FUNCTION_INFO_V1(create_elabel); Datum create_elabel(PG_FUNCTION_ARGS) { - char *graph; - Name graph_name; - char *graph_name_str; + char *graph_name; Oid graph_oid; List *parent; - RangeVar *rv; - - char *label; - Name label_name; - char *label_name_str; + char *label_name; /* checking if user has not provided the graph name */ if (PG_ARGISNULL(0)) @@ -261,41 +256,48 @@ Datum create_elabel(PG_FUNCTION_ARGS) errmsg("label name must not be NULL"))); } - graph_name = PG_GETARG_NAME(0); - label_name = PG_GETARG_NAME(1); + graph_name = PG_GETARG_CSTRING(0); + label_name = PG_GETARG_CSTRING(1); - graph_name_str = NameStr(*graph_name); - label_name_str = NameStr(*label_name); + /* validate the graph and label names */ + if (is_valid_graph_name(graph_name) == 0) + { + ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("graph name is invalid"))); + } + + if (is_valid_label_name(label_name, 0) == 0) + { + ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("label name is invalid"))); + } /* Check if graph does not exist */ - if (!graph_exists(graph_name_str)) + if (!graph_exists(graph_name)) { ereport(ERROR, (errcode(ERRCODE_UNDEFINED_SCHEMA), - errmsg("graph \"%s\" does not exist.", graph_name_str))); + errmsg("graph \"%s\" does not exist.", graph_name))); } - graph_oid = get_graph_oid(graph_name_str); + graph_oid = get_graph_oid(graph_name); /* Check if label with the input name already exists */ - if (label_exists(label_name_str, graph_oid)) + if (label_exists(label_name, graph_oid)) { ereport(ERROR, (errcode(ERRCODE_UNDEFINED_SCHEMA), - errmsg("label \"%s\" already exists", label_name_str))); + errmsg("label \"%s\" already exists", label_name))); } /* Create the default label tables */ - graph = graph_name->data; - label = label_name->data; - - rv = get_label_range_var(graph, graph_oid, AG_DEFAULT_LABEL_EDGE); + rv = get_label_range_var(graph_name, graph_oid, AG_DEFAULT_LABEL_EDGE); parent = list_make1(rv); - create_label(graph, label, LABEL_TYPE_EDGE, parent); + create_label(graph_name, label_name, LABEL_TYPE_EDGE, parent); ereport(NOTICE, - (errmsg("ELabel \"%s\" has been created", NameStr(*label_name)))); + (errmsg("ELabel \"%s\" has been created", label_name))); PG_RETURN_VOID(); } @@ -318,7 +320,7 @@ void create_label(char *graph_name, char *label_name, char label_type, int32 label_id; Oid relation_id; - if (!is_valid_label(label_name, label_type)) + if (!is_valid_label_name(label_name, label_type)) { ereport(ERROR, (errcode(ERRCODE_UNDEFINED_SCHEMA), errmsg("label name is invalid"))); diff --git a/src/backend/utils/name_validation.c b/src/backend/utils/name_validation.c index 2ee998dea..6bfd62f28 100644 --- a/src/backend/utils/name_validation.c +++ b/src/backend/utils/name_validation.c @@ -55,12 +55,17 @@ int is_valid_graph_name(const char *graph_name) * @param label_type label type defined in label_commands.h * @return int */ -int is_valid_label(char *label_name, char label_type) +int is_valid_label_name(char *label_name, char label_type) { int len = strlen(label_name); if (len < MIN_LABEL_NAME_LEN || len > MAX_LABEL_NAME_LEN) { + ereport(WARNING, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("label name length not in range (%d <= length <= %d) length = %d", + MIN_LABEL_NAME_LEN, MAX_LABEL_NAME_LEN, len))); + return 0; } diff --git a/src/include/utils/name_validation.h b/src/include/utils/name_validation.h index 7cfe9bf7b..430da672e 100644 --- a/src/include/utils/name_validation.h +++ b/src/include/utils/name_validation.h @@ -35,10 +35,10 @@ #define MAX_GRAPH_NAME_LEN 63 #define MIN_GRAPH_NAME_LEN 3 -#define MAX_LABEL_NAME_LEN 65535 +#define MAX_LABEL_NAME_LEN NAMEDATALEN -1 #define MIN_LABEL_NAME_LEN 1 int is_valid_graph_name(const char *graph_name); -int is_valid_label(char *label_name, char label_type); +int is_valid_label_name(char *label_name, char label_type); #endif