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

Fix some incorrect memory handling #5317

Merged
merged 2 commits into from
Feb 20, 2023
Merged

Fix some incorrect memory handling #5317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b49910
Fix some incorrect memory handling
Medvecrab Feb 14, 2023
c770a86
Fix changelog message for NameData issue
mkindahl Feb 20, 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
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ accidentally triggering the load of a previous DB version.**

## Unreleased

**Bugfixes**
* #5336 Use NameData and namestrcpy for names
* #5317 Fix some incorrect memory handling

**Thanks**
* @Medvecrab for discovering an issue with copying NameData when forming heap tuples.

## 2.10.0 (2023-02-21)

Expand Down
3 changes: 2 additions & 1 deletion src/process_utility.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1505,7 +1505,8 @@ process_relations_in_namespace(GrantStmt *stmt, Name schema_name, Oid namespaceI

while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
Name relname = &((Form_pg_class) GETSTRUCT(tuple))->relname;
Name relname = palloc(NAMEDATALEN);
namestrcpy(relname, NameStr(((Form_pg_class) GETSTRUCT(tuple))->relname));

/* these are being added for the first time into this list */
process_grant_add_by_name(stmt, false, schema_name, relname);
Expand Down
6 changes: 3 additions & 3 deletions src/ts_catalog/metadata.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ ts_metadata_insert(const char *metadata_key, Datum metadata_value, Oid type,
bool isnull = false;
Catalog *catalog = ts_catalog_get();
Relation rel;
char key_data[NAMEDATALEN];
NameData key_data;

rel = table_open(catalog_get_table_id(catalog, METADATA), ShareRowExclusiveLock);

Expand All @@ -157,10 +157,10 @@ ts_metadata_insert(const char *metadata_key, Datum metadata_value, Oid type,

/* We have to copy the key here because heap_form_tuple will copy NAMEDATALEN
* into the tuple instead of checking length. */
strlcpy(key_data, metadata_key, NAMEDATALEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please remove this part as strlcpy should take care of the trailing zero - I've checked with valgrind and this doesn't cause/fix any issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to defend this change.

Firstly, the first patch and the second patch solve different problems, that are only grouped in one issue because they are were found by valgrind together.

The thing with strlcpy is that it only copies amount of bytes up to length of source string and then terminates the string with zero.
In this case, key_data is a char array the size of NAMEDATALEN (which is 64). And metadata_key that is being copied there isn't always NAMEDATALEN long. And key_data is allocated at the top of the stack, and can contain junk. Or whatever was at the top of the stack. So it's better to fill it it with zeroes and only then copy something there.

For me, valgrind still shows first error stated in #5311, and pageinspect still shows junk. And with patch (initial or the ones linked below) this junk is gone and there are only zeroes.

Reproduction is simple - start postgres under valgrind and execute 'CREATE EXTENSION timescaledb;' with psql.

I can provide at least two alternative patches that will first fill the string with zeroes: see attachements

metadata_memory_fix_memcpy.patch
metadata_memory_fix_namedata.patch

If you'll like any of above patches, I'll replace current code in commit with that patch.

If you're still not convinced and can't reproduce it, I'll remove changes in metadata.c.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing with strlcpy is that it only copies amount of bytes up to length of source string and then terminates the string with zero.
In this case, key_data is a char array the size of NAMEDATALEN (which is 64). And metadata_key that is being copied there isn't always NAMEDATALEN long. And key_data is allocated at the top of the stack, and can contain junk. Or whatever was at the top of the stack. So it's better to fill it it with zeroes and only then copy something there.

Not sure I follow the reasoning here: strlcpy() copies the bytes from source to target and is guaranteed to null-terminate the string as long as the size is larger than 0.

From the manual of strlcpy():

Unlike [strncpy(3) and strncat(3)], strlcpy() and strlcat() take the full size of the buffer (not just the length) and guarantee to NUL-terminate the result (as long as size is larger than 0 ...)

So, if the source string is shorter, it will be copied fully into the target, and if it is longer, just the initial prefix will be copied and the string null-terminated, so it basically does the same as the code that you're replacing it with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, maybe we aren't on the same page here. When manual of strlcpy says 'guarantee to NUL-terminate' it means the following: a sequence of symbols starting at an address with a symbolic name "buffer" will end with '\0', guaranteed. And if the buffer was a char pointer that is supposed to be dynamically allocated, that's fine. But in this case, key_data isn't allocated dynamically. It's an array, size of NAMEDATALEN (64). And it's initially not guaranteed to be empty! When we copy metadata_key (which isn't 100% confirmed to be exactly 64 chars long) into it with strlcpy, we get something like this inside: "metadata_key_value\0junk_symbols_until_64th...". key_data is a char pointer, yes, and if we tried to pass it to, for example, strlen after copying something into it with strlcpy, it would return us it's current length (amount of chars befor '\0'), not 64.

But you say here in the comment (metadata.c, lines 158-159):

/* We have to copy the key here because heap_form_tuple will copy NAMEDATALEN
	 * into the tuple instead of checking length. */

And that means, that any info left after that terminating zero will be pasted onto a page (as can be seen in #5311):

-[ RECORD 3 ]-----------------------------------------------------------------------------------------------------------------------------------------------
----------------------------------------------------------------
lp          | 3
lp_off      | 7816
lp_flags    | 1
lp_len      | 126
t_xmin      | 755
t_xmax      | 0
t_field3    | 0
t_ctid      | (0,3)
t_infomask2 | 3
t_infomask  | 2306
t_hoff      | 24
t_bits      |
t_oid       |
t_data      | \x6578706f727465645f7575696400000000000000000000006822cd0400000000080000000000000010000000000000007827510900000000e0235109000000004b3138303062
3539332d383261362d346365382d623561652d66626235363234666162316401

This can be dangerous, because when we calculate checksums of this page, if this junk if not the same every time, it can be problematic and lead to checksum missmatch.
The simplest solution, in my opinion, is to memset key_data with zeroes before copying anything into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You actually have a very good point: the value for the Name field is a fixed-size char array (effectively, at least), which means that there will be random data added to the tuple.

Note that this has nothing to do with whether the field is dynamically allocated or not, it is just a side-effect of copying NAMEDATALEN bytes from the source.

However, your patch does not solve the problem in that case, since all you do is replicating the behavior of strlcpy with this change.

I agree that a working change would be what you suggest: initialize the key_data variable with zeroes (it is an auto variable, so it is not automatically zero initialized) and then use strlcpy. You need to use strlcpy since you need to guarantee that the string is null-terminated even if metadata_key is too long for the buffer.

Copy link

Choose a reason for hiding this comment

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

Please let me chime in. As strncpy(3) says:

If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written.

So, in general, strncpy and strlcpy are not equivalent. The Postgres function namestrcpy deliberately uses strncpy instead of strlcpy to zero-pad the destination.

void
namestrcpy(Name name, const char *str)
{
	/* NB: We need to zero-pad the destination. */
	strncpy(NameStr(*name), str, NAMEDATALEN);
	NameStr(*name)[NAMEDATALEN - 1] = '\0';
}

I think it would be better to replace strlcpy here with namestrcpy to avoid potential confusion in future (please see the alternative patch metadata_memory_fix_namedata.patch mentioned above). I can imagine someone replacing strncpy with strlcpy again, because it's considered a safer alternative. But they are not really interchangable.

Copy link
Contributor

@mkindahl mkindahl Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please let me chime in. As strncpy(3) says:

If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written.

Thank you, I missed that comment. In that case, moving away from strlcpy is an improvement, but I agree that using namestrcpy is the better alternative.

namestrcpy(&key_data, metadata_key);

/* Insert into the catalog table for persistence */
values[AttrNumberGetAttrOffset(Anum_metadata_key)] = CStringGetDatum(key_data);
values[AttrNumberGetAttrOffset(Anum_metadata_key)] = NameGetDatum(&key_data);
values[AttrNumberGetAttrOffset(Anum_metadata_value)] =
convert_type_to_text(metadata_value, type);
values[AttrNumberGetAttrOffset(Anum_metadata_include_in_telemetry)] =
Expand Down