Disallow string.Replace with a zero-weight target string by GrabYourPitchforks · Pull Request #31946 · dotnet/runtime

GrabYourPitchforks · 2020-02-07T21:58:45Z

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

GrabYourPitchforks · 2020-02-07T21:58:45Z

Fixes#1060

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

GrabYourPitchforks · 2020-02-07T21:58:45Z

Fixes#1060

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

GrabYourPitchforks · 2020-02-07T21:58:45Z

Fixes#1060

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

GrabYourPitchforks · 2020-02-07T21:58:45Z

Fixes#1060

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

GrabYourPitchforks · 2020-02-07T21:58:45Z

Fixes#1060

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

GrabYourPitchforks · 2020-02-07T21:58:45Z

Fixes#1060

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

GrabYourPitchforks · 2020-02-07T21:58:45Z

Fixes#1060

This validates that if the target string in a call to string.Replace has zero collation weight, we'll throw ArgumentException (just as if the target string were empty) rather than enter an infinite loop.

I'm reusing the original exception text rather than creating a new resource string because I don't expect this to be a common scenario at all.

tarekgh · 2020-02-07T22:18:10Z

+ if (matchLength == 0)
+ {
+ throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue));
+ }


After seeing the customer scenario, it may be better to just return from the method without throwing. what you think?

I thought about that, but then why not have string.Replace(string.Empty) also return the original string without throwing?

why not have string.Replace(string.Empty) also return the original string without throwing?
I am ok to make this not throw too. Throwing will require the callers to handle the exceptions and I am not expecting the callers will have the knowledge we can throw in this case.

I'm also fine with making empty target strings not throw, as you had suggested. Would we be able to make this change or would it be considered too breaking?

Would we be able to make this change or would it be considered too breaking?
I think we can document the breaking changes for this case. The breaking scenario will be very limited as we are going not to throw in cases we used to throw before.

GrabYourPitchforks · 2020-02-10T18:56:25Z

After speaking offline with @tarekgh, seems like this might be the best course of action:

Have this PR not throw when given a zero-weight target string. It instead just terminates the replacement loop and returns whatever work has been done so far. This should make it easier to port downlevel.
If we care, send a separate PR to unify the "empty target string" and "zero-weight target string" behaviors so that neither throws. This would affect netcoreapp5.0+ only and would be to provide improved consistency for the API logic.

GrabYourPitchforks · 2020-02-11T04:28:08Z

Looks like System.Threading.Tasks.Tests is failing across the board in CI right now. Unrelated to this PR, so going forward with merging.

When string.Replace is given a target string with zero collation weight, it would enter an infinite loop. It is now changed so that the call to Replace terminates when such a condition is encountered.

Disallow string.Replace with a zero-weight replacement string
f3fb06c

GrabYourPitchforks added the area-System.Runtime label Feb 7, 2020

GrabYourPitchforks requested a review from tarekgh February 7, 2020 21:58

tarekgh reviewed Feb 7, 2020
View reviewed changes

jkotas added area-System.Globalization and removed area-System.Runtime labels Feb 7, 2020

stephentoub approved these changes Feb 8, 2020
View reviewed changes

GrabYourPitchforks added 2 commits February 10, 2020 18:27

Don't fail when given zero-weight target string
aa882c4

Merge remote-tracking branch 'origin/master' into string_replace_2
126051d

tarekgh approved these changes Feb 11, 2020
View reviewed changes

GrabYourPitchforks merged commit 50bd5ba into dotnet:masterFeb 11, 2020

GrabYourPitchforks deleted the string_replace_2 branch February 11, 2020 04:30

GrabYourPitchforks mentioned this pull request Feb 12, 2020
[release/3.1] Port dotnet/runtime#31946 fix for string.Replace infinite loop dotnet/coreclr#28014
Merged

GrabYourPitchforks mentioned this pull request Feb 12, 2020
[release/uwp6.2] Port dotnet/runtime#31946 fix for string.Replace infinite loop dotnet/coreclr#28015
Merged

tarekgh mentioned this pull request May 13, 2020
a call to string.Replace does not return on linux. #36328
Closed

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020