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

Make the history file mutext name unique across sessions #1061

Merged
merged 4 commits into from
Sep 23, 2019

Conversation

daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Sep 20, 2019

GetHistorySaveFileMutexName() uses GetHashCode() to construct the mutex name.
GetHashCode() in .NET Core returns a value that is unique in the current process only, and it results in different mutex name being used for different pwsh sessions for the history file.

The FNV-1a hashing algorithm is used (32-bit) to provide a consistent hash code across different pwsh sessions.

@lzybkr
Copy link
Member

lzybkr commented Sep 20, 2019

Rather than hashing, we might as well just use the filename directly. There is a limit (260) in the mutex name, so if the name is too long, I'd drop the leading part of the filename, it's likely not unique. For example, c:\Users\jason\AppData\ seems perfectly safe to drop and have reasonable uniqueness.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Sep 20, 2019

Backslash \ is a reserved character for mutex name, so using file name means we need extra work to normalize the path separator.
What is the disadvantage of using hashing? It's a simple hashing algorithm. There is possibility of collision, but should be rare.

@lzybkr
Copy link
Member

lzybkr commented Sep 20, 2019

CRC32 is actually pretty bad, I've seen collisions in practice.

But I'm mostly objecting to needing to copy the code and hoping for fewer lines of code.

There is also a benefit to using the filename - it should be more obvious what the mutex is protecting if you were using low level tools like the module NtObjectManager:

#137 PS> Get-NtMutant PSReadLineHistoryFile_1095088493 | fl *

Owner                 : PID: 0 - TID: 0
CurrentCount          : 1
OwnedByCaller         : False
AbandonedState        : False
GrantedAccess         : QueryState, Delete, ReadControl, WriteDac, WriteOwner, Synchronize
FullPath              : \Sessions\1\BaseNamedObjects\PSReadLineHistoryFile_1095088493
GrantedAccessMask     : 2031617
SecurityDescriptor    : O:BAG:DUD:(A;;0x1f0001;;;SY)(A;;0x1f0001;;;BA)(A;;0x1f0001;;;S-1-5-5-0-16814681)
Sddl                  : O:BAG:DUD:(A;;0x1f0001;;;SY)(A;;0x1f0001;;;BA)(A;;0x1f0001;;;S-1-5-5-0-16814681)
Handle                : 0x108C
NtTypeName            : Mutant
NtType                : Name = Mutant - Index = 17
Name                  : PSReadLineHistoryFile_1095088493
CanSynchronize        : True
CreationTime          : 12/31/1600 4:00:00 PM
AttributesFlags       : None
HandleReferenceCount  : 2
PointerReferenceCount : 65491
Inherit               : False
ProtectFromClose      : False
Address               : 0
IsContainer           : False

@daxian-dbw
Copy link
Member Author

OK, got your concern. I will update the PR to not rely on the CRC32 hashing function.

@lzybkr
Copy link
Member

lzybkr commented Sep 23, 2019

If you did want to stick with hashing, I'd use something like fnv-1a instead, it's very little code.

@daxian-dbw
Copy link
Member Author

Yes, personally I still want to use a hash algorithm for the unique name purpose. I will use fnv-1a instead.

@rkeithhill
Copy link
Contributor

I suppose using a generated GUID would be too long?

@daxian-dbw
Copy link
Member Author

The GUID needs to be generated based on the history file path, so that when the path is changed in a pwsh session, a different mutex name is used to guard that file.
Generating a GUID out of a file name is basically hashing again.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Sep 23, 2019

@lzybkr The PR has been updated to use the FNV-1a hashing algorithm. PR description is also updated. Thanks for the suggestion!

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -146,7 +171,7 @@ private string GetHistorySaveFileMutexName()
{
// Return a reasonably unique name - it's not too important as there will rarely
// be any contention.
return "PSReadLineHistoryFile_" + _options.HistorySavePath.GetHashCode();
return "PSReadLineHistoryFile_" + FNV1a32Hash.ComputeHash(_options.HistorySavePath);
Copy link
Member

Choose a reason for hiding this comment

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

I was going to suggest avoiding the conversion to byte array (can we use a span somehow?) but then realized we probably should be normalizing the name anyway, e.g. ensuring consistent case on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

we probably should be normalizing the name anyway

Agreed. Update Set-PSReadLineOption to sort of resolve (not really) PSPath. Also call ToLower before sending the file path to hash method.

avoiding the conversion to byte array (can we use a span somehow?)

This is a fun exercise!
We cannot use span because we need to compile it against .NET 4.6.1, but we don't really need to use a span here.
The update ComputeHash(string input) behaves exactly the same as the ComputeHash(string input) implementation in the following code

internal static uint ComputeHash(byte[] buffer)
{
    uint hash = FNV32_OFFSETBASIS;
    for (int i = 0; i < buffer.Length; i++)
    {
        hash = (hash ^ buffer[i]) * FNV32_PRIME;
    }
    return hash;
}

internal static uint ComputeHash(string input)
{
    return ComputeHash(Encoding.Unicode.GetBytes(input));
}

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.

4 participants