Ldap search crashes on Linux when timeout specified

[jformacek]

This is proposed fix for issue #70648 and #66652 On Linux, search timeout is expressed as LDAP_TIMEVAL instead of int - see source

Thanks, Jiri

[msftbot[bot]]

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014 See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is proposed fix for issue #70648 On Linux, search timeout is expressed as LDAP_TIMEVAL instead of int - see source

Thanks, Jiri

Author:	jformacek
Assignees:	-
Labels:	area-System.DirectoryServices
Milestone:	-

[joperezr]

@jformacek thank you so much for sending out the fix! Would you mind also adding a unit test in:

https://github.com/dotnet/runtime/blob/39d82c99b44db470f43fc12498c1aec082e5c6e2/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs#L15

that uses a timeout and perhaps that validates that the timeout is happening?

[jformacek]

Hello Jose, what I can easily do is to add test for searching with time limit specified, like this:

        [ConditionalFact(nameof(IsLdapConfigurationExist))]
        public void TestSearchWithTimeLimit()
        {
            using LdapConnection connection = GetConnection();

            var searchRequest = new SearchRequest(LdapConfiguration.Configuration.SearchDn, "(objectClass=*)", SearchScope.Subtree);
            searchRequest.TimeLimit = TimeSpan.FromMinutes(1);
            _ = (SearchResponse)connection.SendRequest(searchRequest);
            // Shall succeed
        }

For testing that time limit actually works, I believe I would need help on LDAP server side: server would have to actually timeout when performing some search so as the test is successful - then the test could be something like this:

        [ConditionalFact(nameof(IsLdapConfigurationExist))]
        public void TestSearchWithTimeLimitExceeded()
        {
            using LdapConnection connection = GetConnection();

            DirectoryOperationException ex = Assert.Throws<DirectoryOperationException>(() =>
            {
                var searchRequest = new SearchRequest(LdapConfiguration.Configuration.SearchDn, "(objectClass=*)", SearchScope.Subtree);
                searchRequest.TimeLimit = TimeSpan.FromSeconds(1);
                _ = (SearchResponse)connection.SendRequest(searchRequest);
            });

            Assert.Equal(ResultCode.TimeLimitExceeded, ex.Response.ResultCode);
        }

Tricky part is to achieve that server always times out and returns TimeLimitExceeded result code - are you able to achieve it when running the tests?

Please advise which tests to include - I will commit the tests then.

Thank you, Jiri

[joperezr]

I think the test you pointed out above is good enough. Ideally we want to make sure that the server is timing out, but the fact that the native method now accepts our timelimit argument means we are passing it in the correct form, so we should at least add that in order to validate that this doesn't get regressed in the future. We don't currently have a good story to test against actual ldap servers anyway, so I wouldn't worry for now on getting a test that is actually forcing a timeout here, and instead we should add this once we have a better infrastructure for testing against actual servers.

[joperezr]

It would of course be good to have this test you had before be a theory though, and take different inputs to make sure we have the right behavior, for example:

timelimit = 1 minute
timelimit = 0 seconds
timelimit = -5 seconds

We should just make sure we match the behavior for the second and third cases to what currently happens on Windows.

[jformacek]

Well, on Windows native api, TimeLimit is declared as ULONG; zero means NoLimit. So technically, you cannot pass negative value to Windows API.

On Linux, TimeLimit is pointer to timeval structure; passing NULL is interpreted as NoLimit.

Declarations in LdapPal.[platform] (incorrectly?) use int instead of ulong - so it allows to pass negative value. I did not want to change declarations, so I translate limits less than 1 as NoLimit on Linux (Linux interpretes zero in timeval->tv_sec as invalid value) ==> so I'm not sure if theory with different values as you suggested would be useful as meaning of values is different on each platform

Thoughts?

[joperezr]

Well, SearchRequest.TimeLimit public API is a TimeSpan right? and that is the same for Linux and Windows, so it might be good to add a test with searchRequest.TimeLimit = new Timespan(-10); or something like that and ensuring we have the same behavior in Windows and Linux.

[jformacek]

ok, I added test with negative TimeSpan

Thanks, Jiri

[joperezr]

/azp run runtime

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[t00]

Will this fix be incorporated into a next release of dotnet 6?

[joperezr]

It won’t. This change was merged into .NET 7, so it will be present in .NET 7 but there are no current plans to backport this to .NET 6.

[Miepee]

After the PR was merged, neither of the issues mentioned in the description were closed. Was that intentional?

[joperezr]

It wasn't, thanks for pointing it out. I have closed both issues now.