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

we must also guard mmap'd access in AccessDirectory #185

Merged

Conversation

magibney
Copy link
Collaborator

opening this PR against fs/branch_9_3; will forward-port to fs/branch_9x.

@wiz-inc-465cdd3a04-cowpaths
Copy link

wiz-inc-465cdd3a04-cowpaths bot commented Mar 15, 2024

Wiz Scan Summary

IaC Misconfigurations 0C 0H 0M 0L 0I
Vulnerabilities 0C 0H 0M 0L 0I
Sensitive Data 0C 0H 0M 0L 0I
Total 0C 0H 0M 0L 0I
Secrets 0🔑

@@ -92,6 +92,15 @@ static OpenOption getDirectOpenOption() {
private final boolean useAsyncIO;
private final boolean useDirectIO;

public CompressingDirectory(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excuse me if I missed it but I'm not seeing this new constructor used anywhere, is this necessary now or for something in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a public ctor here in order to make NodeLevelTeeDirectoryState class public (for external use, e.g. in tests and transfer scripts), without having to make the internal fields accessible.

This is a good callout though, let me see if I can adjust this to make it a little more clear how this is supposed to be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated usages to make it clear that the preferred/public ctor is the one that takes NodeLevelTeeDirectoryState arg. This will hopefully make it a little less unusual-looking!

@@ -578,14 +580,20 @@ private static ByteBufferGuard.BufferCleaner unmapHack() {
}
}

static final class LazyLoadInput extends IndexInput implements RandomAccessInput {
public interface LazyLoad {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this interface necessary right now? It looks like we don't reference anything by this interface, and only have one implementation of it. Is this something that's more forward looking, for when we are upstreaming this in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! yeah; this is similar to the above -- this interface is used in testing by external code. I added a comment to this effect in 6fb2dab

Copy link
Collaborator

@nginthfs nginthfs left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@hiteshk25 hiteshk25 left a comment

Choose a reason for hiding this comment

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

LGTM

@hiteshk25 hiteshk25 merged commit b9fd757 into fs/branch_9_3 Mar 18, 2024
7 checks passed
justinrsweeney pushed a commit that referenced this pull request Apr 26, 2024
* we must also guard mmap'd access in AccessDirectory

* tidy

* slightly different way to alter TeeDirectory test path scoping

* preferred public ctor for CompressingDirectory takes NodeLevelTeeDirectoryState

* add comment about CompressingDirectory ctor

* add a comment about seemingly superfluous interface `LazyLoad`
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.

3 participants