Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix sanitizer errors #1266

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix sanitizer errors #1266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de3633d
Fix sanitizer aligment errors with memcpy.
antamel Sep 12, 2023
3afbad6
Fix sanitizer "invalid values for type '_Bool' " errors with palloc0.
antamel Sep 13, 2023
2840ce1
Fix sanitizer heap-buffer-overflow errors during csv files parsing.
antamel Sep 14, 2023
71f445a
Save the metadata representing by the extensible node with the null t…
antamel Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/backend/executor/cypher_set.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ static agtype_value *replace_entity_in_path(agtype_value *path,
agtype *prop_agtype;
int i;

r = palloc(sizeof(agtype_value));
r = palloc0(sizeof(agtype_value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is generally good form to pre-initialize to zero, is it really necessary if the routine that uses it, sets it up properly? In the case of these iterator functions, which are generally slow, it takes up additional cpu cycles. So, I'm not sure this is necessary. Can you justify adding this?


prop_agtype = agtype_value_to_agtype(path);
it = agtype_iterator_init(&prop_agtype->root);
Expand Down
2 changes: 1 addition & 1 deletion src/backend/parser/cypher_clause.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6760,7 +6760,7 @@ static FuncExpr *make_clause_func_expr(char *function_name,
*/
outNode(str, clause_information);

clause_information_const = makeConst(INTERNALOID, -1, InvalidOid, str->len,
clause_information_const = makeConst(INTERNALOID, -1, InvalidOid, str->len + 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please justify why there needs to be an additional +1 to the length?

PointerGetDatum(str->data), false, false);

func_oid = get_ag_func_oid(function_name, 1, INTERNALOID);
Expand Down
2 changes: 1 addition & 1 deletion src/backend/utils/adt/agtype.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9043,7 +9043,7 @@ agtype_value *agtype_composite_to_agtype_value_binary(agtype *a)
errmsg("cannot convert agtype scalar objects to binary agtype_value objects")));
}

result = palloc(sizeof(agtype_value));
result = palloc0(sizeof(agtype_value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is generally good form to pre-initialize to zero, is it really necessary if the routine that uses it, sets it up properly? In the case of these iterator functions, which are generally slow, it takes up additional cpu cycles. So, I'm not sure this is necessary. Can you justify adding this?


// convert the agtype to a binary agtype_value
result->type = AGTV_BINARY;
Expand Down
10 changes: 5 additions & 5 deletions src/backend/utils/adt/agtype_ext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ bool ag_serialize_extended_type(StringInfo buffer, agtentry *agtentry,
/* copy in the int_value data */
numlen = sizeof(int64);
offset = reserve_from_buffer(buffer, numlen);
*((int64 *)(buffer->data + offset)) = scalar_val->val.int_value;
memcpy( buffer->data + offset, &scalar_val->val.int_value, numlen);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this is necessary, maybe it is just me, could you please elaborate?


*agtentry = AGTENTRY_IS_AGTYPE | (padlen + numlen + AGT_HEADER_SIZE);
break;
Expand All @@ -70,7 +70,7 @@ bool ag_serialize_extended_type(StringInfo buffer, agtentry *agtentry,
/* copy in the float_value data */
numlen = sizeof(scalar_val->val.float_value);
offset = reserve_from_buffer(buffer, numlen);
*((float8 *)(buffer->data + offset)) = scalar_val->val.float_value;
memcpy(buffer->data + offset, &scalar_val->val.int_value, numlen);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this is necessary, maybe it is just me, could you please elaborate?


*agtentry = AGTENTRY_IS_AGTYPE | (padlen + numlen + AGT_HEADER_SIZE);
break;
Expand Down Expand Up @@ -156,12 +156,12 @@ void ag_deserialize_extended_type(char *base_addr, uint32 offset,
{
case AGT_HEADER_INTEGER:
result->type = AGTV_INTEGER;
result->val.int_value = *((int64 *)(base + AGT_HEADER_SIZE));
memcpy(&result->val.int_value, base + AGT_HEADER_SIZE, sizeof(int64));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this is necessary, maybe it is just me, could you please elaborate?

break;

case AGT_HEADER_FLOAT:
result->type = AGTV_FLOAT;
result->val.float_value = *((float8 *)(base + AGT_HEADER_SIZE));
memcpy(&result->val.float_value, base + AGT_HEADER_SIZE, sizeof(float8));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this is necessary, maybe it is just me, could you please elaborate?

break;

case AGT_HEADER_VERTEX:
Expand Down Expand Up @@ -195,7 +195,7 @@ static void ag_deserialize_composite(char *base, enum agtype_value_type type,
//offset container by the extended type header
char *container_base = base + AGT_HEADER_SIZE;

r = palloc(sizeof(agtype_value));
r = palloc0(sizeof(agtype_value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is generally good form to pre-initialize to zero, is it really necessary if the routine that uses it, sets it up properly? In the case of these iterator functions, which are generally slow, it takes up additional cpu cycles. So, I'm not sure this is necessary. Can you justify adding this?


it = agtype_iterator_init((agtype_container *)container_base);
while ((tok = agtype_iterator_next(&it, r, true)) != WAGT_DONE)
Expand Down
12 changes: 6 additions & 6 deletions src/backend/utils/adt/agtype_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ agtype_value *find_agtype_value_from_container(agtype_container *container,
return NULL;
}

result = palloc(sizeof(agtype_value));
result = palloc0(sizeof(agtype_value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm just not sure that it is necessary.


if ((flags & AGT_FARRAY) && AGTYPE_CONTAINER_IS_ARRAY(container))
{
Expand Down Expand Up @@ -554,7 +554,7 @@ agtype_value *get_ith_agtype_value_from_container(agtype_container *container,
if (i >= nelements)
return NULL;

result = palloc(sizeof(agtype_value));
result = palloc0(sizeof(agtype_value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm just not sure that it is necessary.


fill_agtype_value(container, i, base_addr, get_agtype_offset(container, i),
result);
Expand Down Expand Up @@ -716,7 +716,7 @@ static agtype_value *push_agtype_value_scalar(agtype_parse_state **pstate,
(*pstate)->size = 4;
}
(*pstate)->cont_val.val.array.elems =
palloc(sizeof(agtype_value) * (*pstate)->size);
palloc0(sizeof(agtype_value) * (*pstate)->size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm just not sure that it is necessary.

(*pstate)->last_updated_value = NULL;
break;
case WAGT_BEGIN_OBJECT:
Expand All @@ -727,7 +727,7 @@ static agtype_value *push_agtype_value_scalar(agtype_parse_state **pstate,
(*pstate)->cont_val.val.object.num_pairs = 0;
(*pstate)->size = 4;
(*pstate)->cont_val.val.object.pairs =
palloc(sizeof(agtype_pair) * (*pstate)->size);
palloc0(sizeof(agtype_pair) * (*pstate)->size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm just not sure that it is necessary.

(*pstate)->last_updated_value = NULL;
break;
case WAGT_KEY:
Expand Down Expand Up @@ -784,7 +784,7 @@ static agtype_value *push_agtype_value_scalar(agtype_parse_state **pstate,
*/
static agtype_parse_state *push_state(agtype_parse_state **pstate)
{
agtype_parse_state *ns = palloc(sizeof(agtype_parse_state));
agtype_parse_state *ns = palloc0(sizeof(agtype_parse_state));

ns->next = *pstate;
return ns;
Expand Down Expand Up @@ -1306,7 +1306,7 @@ bool agtype_deep_contains(agtype_iterator **val, agtype_iterator **m_contained)
uint32 j = 0;

/* Make room for all possible values */
lhs_conts = palloc(sizeof(agtype_value) * num_lhs_elems);
lhs_conts = palloc0(sizeof(agtype_value) * num_lhs_elems);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm just not sure that it is necessary.


for (i = 0; i < num_lhs_elems; i++)
{
Expand Down
2 changes: 1 addition & 1 deletion src/backend/utils/load/ag_load_edges.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ int create_edges_from_csv_file(char *file_path,
struct csv_parser p;
char buf[1024];
size_t bytes_read;
unsigned char options = 0;
unsigned char options = CSV_APPEND_NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please elaborate why. This is not dismissing it, but inquiring why.

csv_edge_reader cr;

if (csv_init(&p, options) != 0)
Expand Down
2 changes: 1 addition & 1 deletion src/backend/utils/load/ag_load_labels.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ int create_labels_from_csv_file(char *file_path,
struct csv_parser p;
char buf[1024];
size_t bytes_read;
unsigned char options = 0;
unsigned char options = CSV_APPEND_NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please elaborate why. This is not dismissing it, but inquiring why.

csv_vertex_reader cr;

if (csv_init(&p, options) != 0)
Expand Down