Fix ber scanf/printf. #51205

sandreenko · 2021-04-14T01:04:00Z

That is an alternative solution for #49105, instead of #50540.

OSX arm64 has a special calling convention for varargs so we can't rely on the old mechanism when we declared methods as non-varargs but the arguments went to the registers and stack slots that were expected by a native vararg callee.

For completeness, I do the change on all platforms so we don't rely on this behavior that is not guaranteed to work on new platforms/os in the future.

Use the vararg calling convention on Windows and non-vararg methods that are provided by OpenLDAP on Unix.

sandreenko · 2021-04-14T08:27:46Z

@joperezr your idea of using OpenLdap specific functions looks promising, it is much easier to do then the native shim, however, I can't find how to encode ber_printf(berElem, 't'), looks like there is no ber_put_tag or something.
that is how it works in ldap:
https://clickhouse.tech/codebrowser/html_report/ClickHouse/contrib/openldap/libraries/liblber/encode.c.html#586
we change ber->ber_tag = va_arg( ap, ber_tag_t ); and return.

If we want to stick to this solution we can do a few things:

change ber->ber_tag on C# side - sounds bad because we will have a dependency on BerElement layout;
use "ber_tag_t" argument that "ber_put" functions have
https://clickhouse.tech/codebrowser/html_report/ClickHouse/contrib/openldap/libraries/liblber/encode.c.html#ber_put_int
so we will save the tag value when we see it on c# side and then pass it to the native function - it would introduce a difference between windows and unix implementation, Windows does not have "get/put" methods so we have to stick to "scanf/printf" there.
The change will be here:

runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/common/BerConverter.cs

Line 40 in 19caa60

else if (fmt == 't' || fmt == 'i' || fmt == 'e')

we will start to save "tag" value in a C# variable and pass it to all calls, Windows will ignore it, Unix will use it.
say that the "t" format is unsupported for printf on Unix, it is not currently supported for scanf, so not a big loss?

I will try to code 2. tomorrow, if we don't extend API and don't allow the users to work with the BerElement directly it should work (otherwise we can lose a tag).

joperezr · 2021-04-14T17:16:13Z

Thank you so much for trying this other approach. I agree with your plan of attempting 2 and if that doesn't work then just going with 3 instead. To be honest we don't really use all of the encoding support fully as we mostly only have it to support the basic scenarios that the communication with ldap requires. We have good unit test coverage on the BerPal, and we have some tests that need to be rujn manually against an ldap server to ensure that the overall connection and manipulation of ldap still works as expected, so I can give you more details on how to run that too.

sandreenko · 2021-04-14T20:05:56Z

/azp run runtime-libraries-coreclr outerloop, runtime-libraries-coreclr outerloop-osx

azure-pipelines · 2021-04-14T20:06:19Z

Azure Pipelines successfully started running 2 pipeline(s).

sandreenko · 2021-04-16T01:32:58Z

/azp run runtime-libraries-coreclr outerloop, runtime-libraries-coreclr outerloop-osx

azure-pipelines · 2021-04-16T01:33:23Z

Azure Pipelines successfully started running 2 pipeline(s).

sandreenko · 2021-04-16T07:00:18Z

The failures are due to #51346, I think this PR is ready for review.
Note I have made many changes in the managed implementation of BER that I am not very familiar with so I was mostly relying on our test coverage. Would be nice if somebody who is familiar with BER could review it.

PTAL @joperezr @ericstj , cc @jkotas , @janvorli

sandreenko · 2021-04-19T18:43:40Z

ping @joperezr @ericstj

joperezr · 2021-04-19T18:49:38Z

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ber.cs

+
+        public static int ber_printf_tag(SafeBerHandle berElement, string format, int tag)
+        {
+            return ber_default_successful_return_code;


Should we add a comment here saying why we are not supporting this? Or I wonder if for this scenario we should falk back to use the varargs func instead so that it works as expected on platforms where they are supported?

I will add a comment:
// Ber Linux tags are passed with the values that they affect, so this function does nothing.

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ber.cs

joperezr · 2021-04-19T19:03:05Z

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ber.cs

-        public static extern int ber_scanf_bitstring(SafeBerHandle berElement, string format, ref IntPtr value, ref int bitLength);
+        public static int ber_printf_berarray(SafeBerHandle berElement, string format, IntPtr value, int tag)
+        {
+            Debug.Assert(format == "v" || format == "V");


Same here, could we just fall back to call the old printf in this case? I know it means that this won't work in the new Macs, but that means we won't regress existing runtimes in case support is added in the future.

I have supported 'v' on Linux (and changed the Windows version to be consistent) by converting one 'v' call to several 'o' calls:

runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/common/BerConverter.cs

Line 121 in 0711327

error = EncodingByteArrayHelper(berElement, tempValue, 'o', tag);

it does not work for 'V' because I have not found an API that takes 'BerVal'.
The definition is:

V Several octet strings. A null-terminated array of struct berval *'s is supplied. Note that a construct like '{V}' is required to get an actual SEQUENCE OF octet strings.

and there is no API that takes a single A null-terminated struct berval *'s

so the only thing that regressed is 'V' encoding for Linux. 't' (encoding, decoding), 'v'(encoding) are supported'; 'V' and 'v' decoding were not supported before these changes.

joperezr · 2021-04-19T19:03:15Z

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ber.cs

-        public static extern int ber_scanf_ptr(SafeBerHandle berElement, string format, ref IntPtr value);
+        public static int ber_scanf_multibytearray(SafeBerHandle berElement, string format, ref IntPtr value)
+        {
+            Debug.Assert(format == "v" || format == "V");


Decode for vv and VV was not supported previously so we don't regress anything here:

runtime/src/libraries/System.DirectoryServices.Protocols/tests/BerConverterTests.cs

Line 124 in acff8f3

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) // vv and VV formats are not supported yet in Linux

yeah I understand that, but what I mean is imagine that in a future version of libldap (if it ever ships) they add support for vv or VV, then the old code would have just worked as the scanf calls would just light up and start working; whereas if we keep it the way you have it we would have to basically make the change manually. TBH I doubt libldap will ship a new future major version with this support so I'm fine keeping it as you have it but just wanted to raise the comment to get your opinion

Does this comment means that "vv" and "VV" are not supported in System.DirectoryServices.Protocols or in OpenLDAP? I thought it was about "System.DirectoryServices.Protocols". I guess it was not supported exactly because vararg calling convention for passing such struct was different so we could not call it even on x64 Unix.

...em.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/common/BerConverter.cs

joperezr · 2021-04-19T19:12:19Z

...em.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/common/BerConverter.cs

+
+                    valueCount++;
+                }
+                else if (fmt == 't')


I'm sure I'm missing something obvious but I'm not understanding why we mvoed the t here. even when unsupported seems to me we are doing sort of the same than we were doing before right?

't' means tag and it applies to the next thing in the string, there is no API that does this, like "ber_put_arg" or something, so I introduced a mechanism on C# side that saves the tag when we see it:

tag = (int)value[valueCount]; tagIsSet = true;

and then pass it to the next value, so the tags are kept working on Linux.

joperezr · 2021-04-20T16:56:52Z

just as an overall comment, things look very good here and I'm happy to see that all of our ber unit tests are passing, but it would be good to run all of our scenario tests against an actual LDAP server before merging just in case.

sandreenko · 2021-04-20T22:29:06Z

but it would be good to run all of our scenario tests against an actual LDAP server before merging just in case.

How could I do this?

sandreenko · 2021-04-26T18:35:57Z

ping @joperezr

joperezr

I have ran the manual tests against Active Directory server in both Windows and Linux and they both passed just fine so I think we can go ahead and merge this.

joperezr · 2021-04-28T21:17:15Z

Thanks a lot for the refactoring work @sandreenko!

sandreenko · 2021-04-28T21:30:29Z

Thanks @joperezr for your help with it!

This reverts commit f664348.

Sergey added 2 commits April 13, 2021 15:54

Reenable the tests.

3215d25

Use __arglist for windows.

552d9dd

dotnet-issue-labeler bot added the area-System.DirectoryServices label Apr 14, 2021

sandreenko force-pushed the fixBerScanfPrintf branch 2 times, most recently from 4ac15ed to 91fce13 Compare April 14, 2021 02:28

sandreenko requested a review from joperezr April 14, 2021 07:56

sandreenko marked this pull request as ready for review April 14, 2021 07:57

sandreenko mentioned this pull request Apr 14, 2021

Add a proxy for ber scanf/printf varargs native methods. #50540

Closed

Use ber_put_*/ber_get_* for Unix

46908e6

sandreenko force-pushed the fixBerScanfPrintf branch from 51bb400 to 46908e6 Compare April 15, 2021 23:52

fix tag

0711327

joperezr reviewed Apr 19, 2021

View reviewed changes

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ber.cs Show resolved Hide resolved

joperezr reviewed Apr 19, 2021

View reviewed changes

...em.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/common/BerConverter.cs Outdated Show resolved Hide resolved

joperezr reviewed Apr 19, 2021

View reviewed changes

fix review.

f3fd828

ericstj assigned joperezr Apr 26, 2021

joperezr approved these changes Apr 28, 2021

View reviewed changes

sandreenko merged commit f664348 into dotnet:main Apr 28, 2021

This was referenced Apr 28, 2021

[macOS-arm64] 55 System.DirectoryServices.Protocols.Tests fail on CoreCLR #49105

Closed

Revert "Add ldap libraries to the docker images. (#7197)" dotnet/arcade#7313

Merged

sandreenko pushed a commit to sandreenko/runtime that referenced this pull request Apr 29, 2021

Revert "Fix ber scanf/printf. (dotnet#51205)"

89fab8f

This reverts commit f664348.

jakobbotsch pushed a commit that referenced this pull request Apr 29, 2021

Revert "Fix ber scanf/printf. (#51205)" (#52038)

a16a9e1

This reverts commit f664348.

sandreenko mentioned this pull request May 3, 2021

Fix ber scanf/printf, fix for a reverted PR. #52178

Merged

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ber scanf/printf. #51205

Fix ber scanf/printf. #51205

sandreenko commented Apr 14, 2021 •

edited

Loading

sandreenko commented Apr 14, 2021 •

edited

Loading

joperezr commented Apr 14, 2021

sandreenko commented Apr 14, 2021

azure-pipelines bot commented Apr 14, 2021

sandreenko commented Apr 16, 2021

azure-pipelines bot commented Apr 16, 2021

sandreenko commented Apr 16, 2021

sandreenko commented Apr 19, 2021

joperezr Apr 19, 2021

sandreenko Apr 19, 2021

joperezr Apr 19, 2021

sandreenko Apr 19, 2021

joperezr Apr 19, 2021

sandreenko Apr 19, 2021

joperezr Apr 20, 2021

sandreenko Apr 20, 2021

joperezr Apr 19, 2021

sandreenko Apr 19, 2021 •

edited

Loading

joperezr commented Apr 20, 2021

sandreenko commented Apr 20, 2021

sandreenko commented Apr 26, 2021

joperezr left a comment

joperezr commented Apr 28, 2021

sandreenko commented Apr 28, 2021

Fix ber scanf/printf. #51205

Fix ber scanf/printf. #51205

Conversation

sandreenko commented Apr 14, 2021 • edited Loading

sandreenko commented Apr 14, 2021 • edited Loading

joperezr commented Apr 14, 2021

sandreenko commented Apr 14, 2021

azure-pipelines bot commented Apr 14, 2021

sandreenko commented Apr 16, 2021

azure-pipelines bot commented Apr 16, 2021

sandreenko commented Apr 16, 2021

sandreenko commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

joperezr commented Apr 20, 2021

sandreenko commented Apr 20, 2021

sandreenko commented Apr 26, 2021

joperezr left a comment

Choose a reason for hiding this comment

joperezr commented Apr 28, 2021

sandreenko commented Apr 28, 2021

sandreenko commented Apr 14, 2021 •

edited

Loading

sandreenko commented Apr 14, 2021 •

edited

Loading

sandreenko Apr 19, 2021 •

edited

Loading