Skip to content

Harden against files with embedded NULL#977

Merged
pfitzseb merged 1 commit intomasterfrom
check-for-invalid-file
Aug 5, 2021
Merged

Harden against files with embedded NULL#977
pfitzseb merged 1 commit intomasterfrom
check-for-invalid-file

Conversation

@davidanthoff
Copy link
Copy Markdown
Member

@davidanthoff davidanthoff added this to the Next Patch milestone Aug 5, 2021
@davidanthoff davidanthoff requested a review from a team August 5, 2021 19:09
Copy link
Copy Markdown
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

Still not sure how this can happen.

@davidanthoff
Copy link
Copy Markdown
Member Author

Just some random piece of code writing an invalid file to disc, right?

@pfitzseb
Copy link
Copy Markdown
Member

pfitzseb commented Aug 5, 2021

Hm, fair. I thought VSCode would handle that case, but I guess JS strings can contain NULLs just fine.

@davidanthoff
Copy link
Copy Markdown
Member Author

This is a load path where we load the file content directly from disc, VSC is only telling us that the file changed on disc, but completely out of the picture when it comes to the file content. That whole design is pretty broken because VS Code now supports workspaces that don't exist on disc at all, and I think they are working on some LSP version where we essentially don't have to use file IO functions in the LS at all but can get all content from the client via the protocol, but not entirely sure what the status on that is.

@pfitzseb
Copy link
Copy Markdown
Member

pfitzseb commented Aug 5, 2021

Oh, I could've just read the read(file, String) line :P Didn't actually realize this happened anywhere...

@pfitzseb pfitzseb merged commit 474638a into master Aug 5, 2021
@davidanthoff davidanthoff deleted the check-for-invalid-file branch August 5, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants