Skip to content

Fix use validator on stack after returning. #91

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix use validator on stack after returning. #91

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions pg_documentdb/src/commands/create_collection_view.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,10 @@ ParseCreateSpec(Datum databaseDatum, pgbson *createSpec, bool *hasSchemaValidati
}
else if (strcmp(key, "validator") == 0)
{
spec->validator = ParseAndGetValidatorSpec(&createIter, "create.validator",
hasSchemaValidationSpec);
bson_value_t *validator = palloc0(sizeof(bson_value_t));
Copy link
Member

Choose a reason for hiding this comment

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

Could we avoid the palloc0 changing ParseAndGetValidatorSpec to take a pointer and assign the value to such pointer?

Then we could call the function like:
ParseAndGetValidatorSpec(&createIter, "create.validator", hasSchemaValidationSpec, &spec->validator)

Copy link
Contributor Author

@buriedpot buriedpot Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your review 😊

Do you mean that we should still allocate validator on stack or we should allocate validator on heap in function ParseAndGetValidatorSpec instead of ParseCreateSpec?

As you can see at create_collection_view.c:154, after function ParseCreateSpec returning, spec->validator will be still used, so I think that using palloc is necessary. I can put the palloc into function ParseAndGetValidatorSpec.

Copy link
Contributor Author

@buriedpot buriedpot Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Or you want to use palloc instead of palloc0?

I think using palloc is indeed more reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that spec in create_collection_view is palloc'd already and not in the stack. So we could just change the signature of the ParseAndGetValidatorSpec function to accept a bson_value_t * instead of returning bson_value_t.

Then it will populate the bson_value_t * passed in with the parsed validator spec.

Then we call is like this:

ParseAndGetValidatorSpec(&createIter, "create.validator", hasSchemaValidationSpec, &spec->validator)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your advice, I will try to make some modifications.

Copy link
Contributor Author

@buriedpot buriedpot Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your review.
The validator field in spec is a pointer, so palloc is inevitable.

I can put palloc in function ParseAndGetValidatorSpec instead of ParseCreateSpec.

The core problem is that the function bson_iter_value will return a pointer which points to a memory area on stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

bson_value_t is a struct right? - so if we have spec's validator be a bson_value_t (instead of a const bson_value_t *) then when calling ParseAndGetValidatorSpec as called above, we can have something like

specValidator = *bson_iter_value(&iterator)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will make some modifications. Thank you for reviewing.

Copy link
Member

Choose a reason for hiding this comment

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

Hi, @buriedpot -- where you able to do the adjustment suggested here?

*validator = *(ParseAndGetValidatorSpec(&createIter, "create.validator",
hasSchemaValidationSpec));
spec->validator = validator;
}
else if (strcmp(key, "validationLevel") == 0)
{
Expand Down