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.

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

Add a lint for the Self tuple struct soundness hole #318

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Jun 1, 2023

See comments and such.

@eeeebbbbrrrr
Copy link
Contributor

This presents an interesting conundrum for existing plrust functions that were compiled before this lint.

A new plrust with this lint will require it of any function it tries to load.

This is our first new lint since initial release and I guess we didn’t quite sort this situation out ahead of time.

I’m sure there’s a path forward here to allow existing functions to load, but it’s late here on the east coast. We can figure it out over the next few days.

@thomcc
Copy link
Contributor Author

thomcc commented Jun 1, 2023

FWIW this pattern is quite rare, to the point where a crater run didn't find any offending patterns across all of crates.io (I've also made it take visibility into account, which should match the version run against crater).

@eeeebbbbrrrr
Copy link
Contributor

This might better demonstrate the problem:

[v15.3][1488067] plrust=# create function existing_func() returns void language plrust as $$ Ok(None) $$;
CREATE FUNCTION
Time: 4013.340 ms (00:04.013)
[v15.3][1488067] plrust=# select existing_func();
 existing_func 
---------------
 
(1 row)

go add a new lint to plrust

[v15.3][1488636] plrust=# select existing_func();
ERROR:  
   0: Function not compiled with required lints: new_lint_please

The question here is how do we do add a new lint without requiring it to have been applied to all existing functions? Do we just ignore the list of required lints for new functions?

@thomcc
Copy link
Contributor Author

thomcc commented Jun 1, 2023

Yeah, I see. The issue isn't that code exists that would suddenly fail to compile, the issue is that this lint now exists and maybe we want to recompile existing functions to ensure they don't violate it.

I don't have strong feelings but I think that's a step too painful.

@eeeebbbbrrrr
Copy link
Contributor

Yeah, I see. The issue isn't that code exists that would suddenly fail to compile, the issue is that this lint now exists and maybe we want to recompile existing functions to ensure they don't violate it.

Exactly.

I don't have strong feelings but I think that's a step too painful.

I think it's painful too. Also, plrust doesn't even have a "recompile" facility -- users would need to CREATE OR REPLACE FUNCTION every LANGUAGE plrust function.

Or we (somehow) do that automatically during the extension version upgrade script, but that has a bunch of its own problems.

@thomcc thomcc merged commit bb275cc into main Jun 1, 2023
@thomcc thomcc deleted the thomcc/tuple-struct-self-lint branch June 1, 2023 15:33
thomcc added a commit that referenced this pull request Jun 1, 2023
@thomcc thomcc restored the thomcc/tuple-struct-self-lint branch June 1, 2023 15:35
@thomcc thomcc deleted the thomcc/tuple-struct-self-lint branch June 1, 2023 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants