[System.DirectoryServices.Protocols] Fix ProtocolVersion Overflow on Big Endian Systems

[medhatiwari]

Fixes #112390

This PR fixes an OverflowException in LdapSessionOptions.ProtocolVersion on Big Endian systems (e.g., s390x). The issue occurs because IntPtr.ToInt32() is used to convert a 64-bit pointer to 32-bit, but on Big Endian architectures, the upper 32 bits are nonzero, causing an overflow.

[huoyaoyuan]

It's suboptimal to use byte arrays for endianness correction.

The bug seems to be in the library OpenLdap, which is invoked by ldap_get_option_ptr.

To workaround the issue in managed code, BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value) is the optimal approach, but there may be other endianness-related bug not covered by test.

[medhatiwari]

@huoyaoyuan Thanks for your suggestion! However, using BinaryPrimitives.ReverseEndianness(value) directly won't work in this scenario because IntPtr represents a memory address, not an integer value.

If we apply ReverseEndianness on IntPtr, we are swapping the bytes of the memory address, not the stored integer. This leads to garbage values rather than correctly handling the endianness of the stored integer.

[tmds]

https://linux.die.net/man/3/ldap_get_option says:

LDAP_OPT_PROTOCOL_VERSION
Sets/gets the protocol version. outvalue and invalue must be int *.

Perhaps this should use GetIntValueHelper/SetIntValueHelper instead of GetPtrValueHelper/SetPtrValueHelper?

[medhatiwari]

@tmds
Thanks for the suggestion! Since LDAP_OPT_PROTOCOL_VERSION is expected to be an int* rather than a generic void*, using GetIntValueHelper/SetIntValueHelper instead of GetPtrValueHelper/SetPtrValueHelper makes sense. I'll try this approach and verify whether it resolves the issue correctly without introducing any regressions. If everything works as expected, I'll update the PR accordingly.

[saitama951]

@huoyaoyuan To my understading the IntPtr uses a nint hence that would be a System.Int64 (i.e long) for s390x machines. when doing a ldap_get_option (link) it expects an int* as correctly pointed out by @tmds.

I have tried the following sample OpenLdap program with int and long

// gcc a.c -lldap
int main() {
    LDAP *ld;
    //long version; //output: -1685089024
    int version; //output: 2

    int ret = ldap_initialize(&ld, "ldap://localhost");
    if (ret != LDAP_SUCCESS) {
        fprintf(stderr, "ldap_initialize() failed: %s\n", ldap_err2string(ret));
        return EXIT_FAILURE;
    }

    ret = ldap_get_option(ld, LDAP_OPT_PROTOCOL_VERSION, &version);
    if (ret != LDAP_SUCCESS) {
        ldap_unbind_ext(ld, NULL, NULL);
        return EXIT_FAILURE;
    }

    printf("LDAP Protocol Version: %d\n", version);

    ldap_unbind_ext(ld, NULL, NULL);
    return EXIT_SUCCESS;
}

[steveharter]

I believe the original implementation for Linux used IntPtr and GetPtrValueHelper as a native int since the C APIs use native ints, meaning the int value could be 32- or 64-bits. This was before we had the nint alias to IntPtr. In this case, IntPtr is a number (e.g. "3" for protocol version 3), not an address.

So if the native value is big-endian I believe we need to convert that to little-endian plus only use 32 bits signed since the return value here is Int32.

Note the helpers for Get\SetIntValueHelper and Get\SetPtrValueHelper do the conversion from the value being get\set to a pointer referencing the value which the underlying LDAP expects.

[steveharter]

@tannergooding what are the latest helpers for converting a native big endian int (e.g. ref IntPtr) to Int32?

[tmds]

The native library API expects a pointer to an int (see #112391 (comment)). By calling GetIntValueHelper that is what we're giving it. There is no need to deal with endianness. The endianness for the native library and .NET is the same (for int, IntPtr, ...).

[steveharter]

That is a C API where I assume the native int can be either big- or little-endian and either 32- or 64-bit.

[tmds]

The native int is 32-bit in the machine endianness. The machine endianness is big or little endian, but whatever it is, it is the same for the C library and .NET.

Suppose the int is 1
On a little endian system in memory that is 01 00 00 00
On a big endian system in memory that is 00 00 00 01

Let's assume the next 4 bytes in memory are also zero.

If we're reading this as a 64-bit value (which is what the code was doing before).
On little endian 01 00 00 00 00 00 00 00 is still 1.
On big endian 00 00 00 01 00 00 00 00 is 1 << 32.

The latter is resulting in OverflowException in IntPtr.ToInt32().

[tmds]

This using is no longer needed.

[medhatiwari]

that's done!
Thanks @tmds

[huoyaoyuan]

Nit: no spacing between property accessors.

[medhatiwari]

Nit: no spacing between property accessors.

removed the extra spaces

[medhatiwari]

@medhatiwari please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

@dotnet-policy-service agree company="IBM"

[steveharter]

@medhatiwari can you update your branch please - there are lots of test failures that don't seem to be occurring in more recent PRs. I re-ran the failed tests earlier and still encountering a bunch of failures (see Build Analysis). Thanks

[medhatiwari]

@medhatiwari can you update your branch please - there are lots of test failures that don't seem to be occurring in more recent PRs. I re-ran the failed tests earlier and still encountering a bunch of failures (see Build Analysis). Thanks

@steveharter I have merged the latest main branch into ldapfix. The tests should now be up-to-date.

[medhatiwari]

If everything looks good, can this PR be merged?