[mono] Fix assembly name parser to accommodate non-ASCII UTF8 strings #103363

fanyang-mono · 2024-06-12T18:20:52Z

Manually tested that the testcase failed in the above issue now passed with this change.

Ideally, Mono should share the same assembly name parsing logic as CoreCLR, which is AssemblyNameParser.TryParse.

lewing · 2024-06-12T18:37:33Z

src/mono/mono/metadata/reflection.c

@@ -1548,7 +1548,7 @@ assembly_name_to_aname (MonoAssemblyName *assembly, char *p)
 	}
 	assembly->name = p;
 	s = p;
-	while (*p && (isalnum (*p) || *p == '.' || *p == '-' || *p == '_' || *p == '$' || *p == '@' || g_ascii_isspace (*p)))
+	while (*p && (*p != ','))
 		p++;
 	if (quoted) {
 		if (*p != '"')


It looks like the quoted path will fail now?

What does quoted path look like?

looks like there are some examples here https://github.com/adamsitnik/runtime/blob/b57b100933e306242ae422b9995b47fb3d778dbd/src/libraries/System.Runtime/tests/System.Reflection.Tests/AssemblyNameTests.cs#L37

but for the purposes of my comment I was just pointing out that if the name was qoted your change would walk to the , not the " then return 1 when if (*p != '"') was true

yea. @fanyang-mono I would suggest doing two separate loops for quoted and !quoted. if it's quoted, just take everything until the next close quote (I'm not sure if there's a possibility of escaping a quote). if !quoted, go until the first comma.

I can't actually find anything in the docs about quoted assembly names or about escaping quotes.

https://learn.microsoft.com/en-us/dotnet/api/system.reflection.assemblyname?view=net-8.0#remarks
https://learn.microsoft.com/en-us/dotnet/api/system.type.assemblyqualifiedname?view=net-8.0#remarks

Maybe this is some mono invention, or maybe it's backward compat for .NET Framework? need to check what CoreCLR does

Also, just for clarity if you really want to walk character by character p += g_utf8_jump_table[*p]; is how you could do it here or with more validation like https://github.com/adamsitnik/runtime/blob/4246ba19bd196c5f374d94e5c1fc7b21d53bd9fc/src/mono/mono/eglib/gutf8.c#L106-L113

looks like CoreCLR these days AssemblyNameParser and TryGetNextToken does try to handle single and double quotes and escape characters:

runtime/src/libraries/Common/src/System/Reflection/AssemblyNameParser.cs

Lines 433 to 482 in 1ddfd18

if (quoteChar != 0 && c == quoteChar)

break; // Terminate: Found closing quote of quoted string.

if (quoteChar == 0 && (c is ',' or '='))

{

_index--;

break; // Terminate: Found start of a new ',' or '=' token.

}

if (quoteChar == 0 && (c is '\'' or '\"'))

{

token = default;

return false;

}

if (c is '\\')

{

if (!TryGetNextChar(out c))

{

token = default;

return false;

}

switch (c)

{

case '\\':

case ',':

case '=':

case '\'':

case '"':

sb.Append(c);

break;

case 't':

sb.Append('\t');

break;

case 'r':

sb.Append('\r');

break;

case 'n':

sb.Append('\n');

break;

default:

token = default;

return false;

}

}

else

{

sb.Append(c);

}

Would be nice to share this logic between Mono and CoreCLR somehow.

I think we can probably share for most uses in reflection. The runtime also calls this internally from some places, and also I think the AOT compiler might need it. One idea is to compile the managed TypeNameParser as a library using NativeAOT and call it from the AOT compiler.

kasperk81 · 2024-06-12T19:47:48Z

any tests? enable some of these https://github.com/search?q=repo%3Adotnet%2Fruntime%2045033&type=code

steveisok · 2024-06-13T16:33:50Z

/azp run runtime-ioslike runtime-android runtime-androidemulator

azure-pipelines · 2024-06-13T16:33:56Z

No pipelines are associated with this pull request.

steveisok · 2024-06-13T16:34:16Z

/azp run runtime-ioslike

steveisok · 2024-06-13T16:34:55Z

/azp run runtime-android

azure-pipelines · 2024-06-13T16:35:04Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok · 2024-06-13T16:35:45Z

/azp run runtime-ioslike

azure-pipelines · 2024-06-13T16:35:55Z

Azure Pipelines successfully started running 1 pipeline(s).

src/mono/mono/metadata/reflection.c

steveisok · 2024-06-13T18:57:23Z

/azp run runtime-ioslike

steveisok · 2024-06-13T18:57:31Z

/azp run runtime-android

azure-pipelines · 2024-06-13T18:57:33Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-06-13T18:57:39Z

Azure Pipelines successfully started running 1 pipeline(s).

fanyang-mono · 2024-06-13T20:24:54Z

/azp run runtime-ioslike

fanyang-mono · 2024-06-13T20:25:03Z

/azp run runtime-android

azure-pipelines · 2024-06-13T20:25:04Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-06-13T20:25:32Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok · 2024-06-14T00:50:46Z

/ba-g The failures were leg timeouts that are hard to ping good error messages down to.

steveisok · 2024-06-14T00:52:56Z

/backport to release/8.0-staging

github-actions · 2024-06-14T00:53:09Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/9508872460

github-actions · 2024-06-14T00:54:44Z

@steveisok backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix assembly name parser
Applying: Handle quoted case
Applying: Add utf8 validation
error: sha1 information is lacking or useless (src/mono/mono/metadata/reflection.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0003 Add utf8 validation
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-06-14T00:54:44Z

@steveisok an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

fanyang-mono · 2024-06-14T03:02:49Z

/backport to release/8.0

github-actions · 2024-06-14T03:03:00Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/9509965297

github-actions · 2024-06-14T03:04:34Z

@fanyang-mono backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix assembly name parser
Applying: Handle quoted case
Applying: Add utf8 validation
error: sha1 information is lacking or useless (src/mono/mono/metadata/reflection.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0003 Add utf8 validation
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-06-14T03:04:35Z

@fanyang-mono an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

Fix assembly name parser

ac56cb6

fanyang-mono requested review from lambdageek and thaystg as code owners June 12, 2024 18:20

dotnet-issue-labeler bot added the area-VM-meta-mono label Jun 12, 2024

dotnet-policy-service bot assigned fanyang-mono Jun 12, 2024

lewing reviewed Jun 12, 2024

View reviewed changes

This was referenced Jun 12, 2024

[browser] System.Buffers.ArrayPool.Tests.ArrayPoolUnitTests fails with out of memory #101436

Closed

System.Buffers.Tests timeout on wasm MT #102040

Open

[browser][MT] Tests timed out #102559

Closed

Handle quoted case

f06e478

This was referenced Jun 13, 2024

MT-System.Threading.Tests timeout #102664

Closed

Wasm test failure: System addon update list error SyntaxError: XMLHttpRequest.open #102955

Closed

x64 test failure: DeserializeStoredObjects strings differ. #103367

Closed

Add utf8 validation

2909346

fanyang-mono requested a review from steveisok as a code owner June 13, 2024 15:44

lambdageek reviewed Jun 13, 2024

View reviewed changes

src/mono/mono/metadata/reflection.c Show resolved Hide resolved

Update inptr and handle end of string character

1b7d5c7

lambdageek approved these changes Jun 13, 2024

View reviewed changes

Add test

c5e6a0b

lambdageek approved these changes Jun 13, 2024

View reviewed changes

steveisok approved these changes Jun 13, 2024

View reviewed changes

This was referenced Jun 14, 2024

System.Net.Security.Tests.SslStreamTlsResumeTests.ClientDisableTlsResume_Succeeds failing on windows #103409

Closed

ProcessThreadTests.TestStartTimeProperty failed in CI #103448

Closed

steveisok merged commit 39c4905 into dotnet:main Jun 14, 2024
117 of 129 checks passed

build-analysis bot mentioned this pull request Jun 14, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

fanyang-mono mentioned this pull request Jun 14, 2024

[release/8.0-staging] [mono] Fix assembly name parser to accommodate non-ASCII UTF8 strings #103475

Merged

github-actions bot locked and limited conversation to collaborators Jul 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] Fix assembly name parser to accommodate non-ASCII UTF8 strings #103363

[mono] Fix assembly name parser to accommodate non-ASCII UTF8 strings #103363

fanyang-mono commented Jun 12, 2024 •

edited

Loading

lewing Jun 12, 2024

fanyang-mono Jun 12, 2024 •

edited

Loading

lewing Jun 12, 2024 •

edited

Loading

lambdageek Jun 12, 2024

lewing Jun 12, 2024

lambdageek Jun 12, 2024

fanyang-mono Jun 13, 2024 •

edited

Loading

lambdageek Jun 13, 2024

kasperk81 commented Jun 12, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 13, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 13, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

fanyang-mono commented Jun 13, 2024

fanyang-mono commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 14, 2024

steveisok commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

fanyang-mono commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

	if (quoteChar != 0 && c == quoteChar)
	break; // Terminate: Found closing quote of quoted string.

	if (quoteChar == 0 && (c is ',' or '='))
	{
	_index--;
	break; // Terminate: Found start of a new ',' or '=' token.
	}

	if (quoteChar == 0 && (c is '\'' or '\"'))
	{
	token = default;
	return false;
	}

	if (c is '\\')
	{
	if (!TryGetNextChar(out c))
	{
	token = default;
	return false;
	}

	switch (c)
	{
	case '\\':
	case ',':
	case '=':
	case '\'':
	case '"':
	sb.Append(c);
	break;
	case 't':
	sb.Append('\t');
	break;
	case 'r':
	sb.Append('\r');
	break;
	case 'n':
	sb.Append('\n');
	break;
	default:
	token = default;
	return false;
	}
	}
	else
	{
	sb.Append(c);
	}

[mono] Fix assembly name parser to accommodate non-ASCII UTF8 strings #103363

[mono] Fix assembly name parser to accommodate non-ASCII UTF8 strings #103363

Conversation

fanyang-mono commented Jun 12, 2024 • edited Loading

lewing Jun 12, 2024

Choose a reason for hiding this comment

fanyang-mono Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

lewing Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

lambdageek Jun 12, 2024

Choose a reason for hiding this comment

lewing Jun 12, 2024

Choose a reason for hiding this comment

lambdageek Jun 12, 2024

Choose a reason for hiding this comment

fanyang-mono Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

lambdageek Jun 13, 2024

Choose a reason for hiding this comment

kasperk81 commented Jun 12, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 13, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 13, 2024

steveisok commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

fanyang-mono commented Jun 13, 2024

fanyang-mono commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

azure-pipelines bot commented Jun 13, 2024

steveisok commented Jun 14, 2024

steveisok commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

fanyang-mono commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 14, 2024

fanyang-mono commented Jun 12, 2024 •

edited

Loading

fanyang-mono Jun 12, 2024 •

edited

Loading

lewing Jun 12, 2024 •

edited

Loading

fanyang-mono Jun 13, 2024 •

edited

Loading