`string, params object[]` parameter sanity checking

[jonpryor]

(From the "Save me from myself" department…)

Describe the problem you are trying to solve

C# params arrays are awesome:

partial class Utilities {
    public static void LogError(UsefulInfo info, string format, params object[] args)
    {
        string message = string.Format(format, args);
        // Do something with `message`…
    }
}

They're also a footgun, lying in wait:

class App {
    static UsefulInfo info = …

    public static void Whatever()
    {
        try {
            DoSomething(…);
        }
        catch (Exception e) {
            Utilities.LogError(info, e.ToString());
        }
    }
}

Quick, what's wrong with the above code? It would compile (given enough extra code), and it certainly looks reasonable, and similar code constructs have passed code review innumerable times.

What Could Possibly Go Wrong?

static void DoSomething()
{
    throw new Exception("{tag-name} lol");
}

What happens is the above sketch eventually effectively calls:

string.Format("{tag-name} lol");
// -or-
string.Format("{tag-name} lol", new object[0]);

which promptly throws an exception:

System.FormatException: Input string was not in a correct format.
  at System.Text.StringBuilder.AppendFormatHelper (System.IFormatProvider provider, System.String format, System.ParamsArray args)
  at System.String.FormatHelper (System.IFormatProvider provider, System.String format, System.ParamsArray args)
  at System.String.Format (System.String format, System.Object[] args)

There are (at least) two mitigations for this: overloading, and using the format string.

Overloading: Method overloading nicely "solves"/"avoids" the problem:

partial class Utilities {
    public static void LogError(UsefulInfo info, string message) => … // doesn't call string.Format()
}

The callsite of Utilities.LogError(info, e.ToString()) is unchanged, but now avoids calling string.Format(), and thus avoids the FormatException.

Explicit format strings: occasionally method overloading isn't (easily) possible, in which case the format string should be explicitly specified:

Utilities.LogError(info, "{0}", e.ToString());

Describe suggestions on how to achieve the rule

When an analyzer rule encounters a method invocation:

1. Does the resolved method to-be-called end in a string, params *whatever*[] parameter "pair"? If it does, continue to (2).
2. Are there any values in the params *whatever*[] parameter? If there are, then it's "fine". (It might not be fine; see also CA2241 - Possibility to support custom string formatting methods? roslyn-analyzers#2799! But it's not what this issue is trying to look for.). If there are no values in the params object[] parameter, continue to (3).
3. Issue a warning that the method invocation is potentially problematic.

Additional context

[mavasani]

IMO, this can potentially lead to false positives unless the analysis can confirm that the second last string parameter is being passed as format string to some string.Format call and the last params array parameter is passed as format arguments to that call.

[jonpryor]

@mavasani wrote:

IMO, this can potentially lead to false positives

It will absolutely lead to false positives. I would personally take that tradeoff. I have been bit by this in production more than once, which is too often.

unless the analysis can confirm that the second last string parameter is being passed as format string to some string.Format call and the last params array parameter is passed as format arguments to that call

would result in versioning brittleness: in v1 of the lib it doesn't use string.Format(), but in v2 it does use string.Format(). What used to be fine is now potentially problematic.

The one false positive I'm less certain about is Microsoft.Extensions.Logging, in which there are non-overloaded extension methods which end in string, params object[] such as LoggerExtensions.Log(ILogger, LogLevel, string, params object?[]), but passing a message string containing { doesn't result in any runtime error; in fact, that's one of their examples!

logger.LogInformation("Hello World! Logging is {Description}.", "fun");

[mavasani]

It will absolutely lead to false positives. I would personally take that tradeoff. I have been bit by this in production more than once, which is too often.

Moving to dotnet\runtime for triaging the analyzer proposal. Normally, analyzer suggestions that can lead to many false positives are recommended to be implemented in a separate third party analyzer package, but I will let the triage team make a call.

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

Issue Details

---

(From the "Save me from myself" department…)

Describe the problem you are trying to solve

C# params arrays are awesome:

partial class Utilities {
    public static void LogError(UsefulInfo info, string format, params object[] args)
    {
        string message = string.Format(format, args);
        // Do something with `message`…
    }
}

They're also a footgun, lying in wait:

class App {
    static UsefulInfo info = …

    public static void Whatever()
    {
        try {
            DoSomething(…);
        }
        catch (Exception e) {
            Utilities.LogError(info, e.ToString());
        }
    }
}

Quick, what's wrong with the above code? It would compile (given enough extra code), and it certainly looks reasonable, and similar code constructs have passed code review innumerable times.

What Could Possibly Go Wrong?

static void DoSomething()
{
    throw new Exception("{tag-name} lol");
}

What happens is the above sketch eventually effectively calls:

string.Format("{tag-name} lol");
// -or-
string.Format("{tag-name} lol", new object[0]);

which promptly throws an exception:

System.FormatException: Input string was not in a correct format.
  at System.Text.StringBuilder.AppendFormatHelper (System.IFormatProvider provider, System.String format, System.ParamsArray args)
  at System.String.FormatHelper (System.IFormatProvider provider, System.String format, System.ParamsArray args)
  at System.String.Format (System.String format, System.Object[] args)

There are (at least) two mitigations for this: overloading, and using the format string.

Overloading: Method overloading nicely "solves"/"avoids" the problem:

partial class Utilities {
    public static void LogError(UsefulInfo info, string message) => … // doesn't call string.Format()
}

The callsite of Utilities.LogError(info, e.ToString()) is unchanged, but now avoids calling string.Format(), and thus avoids the FormatException.

Explicit format strings: occasionally method overloading isn't (easily) possible, in which case the format string should be explicitly specified:

Utilities.LogError(info, "{0}", e.ToString());

Describe suggestions on how to achieve the rule

When an analyzer rule encounters a method invocation:

1. Does the resolved method to-be-called end in a string, params *whatever*[] parameter "pair"? If it does, continue to (2).
2. Are there any values in the params *whatever*[] parameter? If there are, then it's "fine". (It might not be fine; see also CA2241 - Possibility to support custom string formatting methods? roslyn-analyzers#2799! But it's not what this issue is trying to look for.). If there are no values in the params object[] parameter, continue to (3).
3. Issue a warning that the method invocation is potentially problematic.

Additional context

Author:	jonpryor
Assignees:	-
Labels:	area-System.Runtime, untriaged, needs-area-label
Milestone:	-