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.
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
Fix sanitizer errors #1266
base: master
Are you sure you want to change the base?
Fix sanitizer errors #1266
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of these changes appear unnecessary and need justification before being considered and added. This is not to say that they won't be added, they just need to be discussed first.
@@ -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; |
There was a problem hiding this comment.
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.
@@ -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; |
There was a problem hiding this comment.
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.
@@ -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)); |
There was a problem hiding this comment.
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?
@@ -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)); |
There was a problem hiding this comment.
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?
@@ -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)); |
There was a problem hiding this comment.
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?
@@ -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); |
There was a problem hiding this comment.
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.
@@ -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); |
There was a problem hiding this comment.
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?
@@ -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); |
There was a problem hiding this comment.
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?
@@ -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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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?
+1 for the palloc0 changes as PostgreSQL recommends it as well: https://www.postgresql.org/docs/current/xfunc-c.html
|
This PR is stale because it has been open 45 days with no activity. Remove "Abondoned" label or comment or this will be closed in 7 days. |
@rafsun42 As the original creator just posted this PR and has yet to respond since, should we break this into 2 PRs, one for memcpy, one for palloc0, attribute this PR and implement ourselves? |
@jrgemignani Sounds good. I am fine with doing either of palloc0 or memcpy. |
Hello!
In the build with address and undefined behavior sanitizers, my colleague
Andrew Bille andrewbille@gmail.com found several errors
in the current master branch.
The build was made with options like that:
There are several "load of misaligned address" sanitizer errors in the log
during installcheck tests with PG at REL_15_STABLE (6db384f2f9).
See master-at-a04fd69b.zip attached. (Other errors are masked by this, about them below.)
Some of these errors are not related to age, is a bug of the sanitizer itself
(see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111382).
The other ones i tried to fix with the Fix sanitizer aligment errors with memcpy. commit.
After that "misaligned" errors disappeared and other errors became visible.
Please see postmaster.log in the after-first-fix-at-de3633d2.zip.
The second commit Fix sanitizer "invalid values for type '_Bool' " errors with palloc0. fixes
runtime errors: "load of value xxx, which is not a valid value for type '_Bool'"
while the third Fix sanitizer heap-buffer-overflow errors during csv files parsing. one
fixes "heap-buffer-overflow on address.." one during csv file parsing.
On applying the first three patches, another error heap-buffer-overflow
became visible. See log in before-last-fix-at-2840ce1f.zip.
The last commit Save the metadata representing by the extensible node with the null t…
fixes it add all tests began to pass without errors with
REL_15_STABLE and REL_14_STABLE branches.
Would be happy for any comments and concerns.
With kind regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company