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.

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

Jump to bottom

Fix incorrect string length calculation #76127

Merged
merged 3 commits into from
Sep 27, 2022
Merged

Fix incorrect string length calculation #76127

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9098be3
Fix incorrect string length calculation
Sparin Sep 24, 2022
d6d3d98
Simplify calculation of null terminated string length from pointer
Sparin Sep 24, 2022
9d4f0dd
Remove StringToPtr(string, out int) overload
Sparin Sep 24, 2022
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
23 changes: 21 additions & 2 deletions ...rectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/LdapPal.Linux.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ internal static int BindToDirectory(ConnectionHandle ld, string who, string pass
IntPtr passwordPtr = IntPtr.Zero;
try
{
passwordPtr = LdapPal.StringToPtr(passwd);
passwordPtr = LdapPal.StringToPtr(passwd, out int length);
BerVal passwordBerval = new BerVal
{
bv_len = passwd?.Length ?? 0,
bv_len = length,
bv_val = passwordPtr,
};

Expand Down Expand Up @@ -177,6 +177,25 @@ internal static int StartTls(ConnectionHandle ldapHandle, ref int serverReturnVa

internal static IntPtr StringToPtr(string s) => Marshal.StringToHGlobalAnsi(s);

internal static unsafe IntPtr StringToPtr(string s, out int length)
{
var pointer = StringToPtr(s);
length = 0;

if (pointer == IntPtr.Zero)
{
return pointer;
}

byte* stringPtr = (byte*)pointer;
while (stringPtr[length] != '\0')
{
length++;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If StringToPtr is not used elsewhere, you can shorten the method to this. Either way a while loop to find the NUL is not advised because it won't be vectorized.

Suggested change
internal static IntPtr StringToPtr(string s) => Marshal.StringToHGlobalAnsi(s);
internal static unsafe IntPtr StringToPtr(string s, out int length)
{
var pointer = StringToPtr(s);
length = 0;
if (pointer == IntPtr.Zero)
{
return pointer;
}
byte* stringPtr = (byte*)pointer;
while (stringPtr[length] != '\0')
{
length++;
}
internal static unsafe IntPtr StringToPtr(string s, out int length)
{
var pointer = Marshal.StringToHGlobalAnsi(s);
length = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(pointer).Length;

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! I didn't know about this before. Applied this suggestion partly as a new commit.
Also, StringToPtr is used in other methods but they don't use string length. So that's why I added this overload with out int length

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing I thought is to not add this new overload and instead calculate the length at the callsite where it's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like that? https://github.com/Sparin/runtime/commit/78484539924277e2295756616a639d1d55c14f95
Tell me if I should apply this commit to PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. It's up to you if you like it more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Seems cleaner now for me than the first one :D


return pointer;
}

/// <summary>
/// Function that will be sent to the Sasl interactive bind procedure which will resolve all Sasl challenges
/// that get passed in by using the defaults that we get passed in.
Expand Down