Protobuf: C# ToString() cannot handle bidirectional references, causing a stack overflow and secure data leakage

[zaneclaes]

What version of protobuf and what language are you using?
Version: v3.5.1 (NuGet official)
Language: C#

What operating system (Linux, Windows, ...) and version?
OSX, Windows

What runtime / compiler are you using (e.g., python version or gcc version)
.NET 4.5 w/ JetBrains Rider IDE

What did you do?
Steps to reproduce the behavior:

1. Create two protobufs messages which have a bidirectional reference, i.e.:

message ParentMessage {
  ChildMessage child = 1;
}

message ChildMessage {
  ParentMessage parent = 1;
}

1. Assign the two messages to reference each other:

ParentMessage parent = new ParentMessage();
ChildMessage child = new ChildMessage() { Parent = parent };
parent.Child = child;

1. Attempt to print one of the objects:
   Console.WriteLine($"Parent: {parent}");

Note the stack-overflow and crash. Also note that there is no way around the bug. Sealed partial classes cannot override the ToString() implementation. Also note that the ToString() implementation may not match with the developer's needs. ToString() is often used by debuggers, loggers, etc. and implementing it in a way which cannot be changed deprives developers of an essential feature necessary for use in a business-grade production-safe environment. For example: the protobuf message might include PII (i.e., passwords) which should not be placed into a log file. Libraries relying upon standard introspection features will inevitably end up logging sensitive data, making Protobuf unusable under these sorts of standard security constraints.

What did you expect to see
At least some built-in recursion detection. Preferably a pluggable way to replace the built-in serialization methods.

What did you see instead?
See above.

Anything else we should know about your project / environment
N/A

[xfxyjwf]

If you intentionally create cyclic references in the class structure, there isn't nothing the implementation can do. It's just not supported.

[zaneclaes]

@xfxyjwf (1) of course there's something the implementation can do. It's called recursion detection. I've personally implemented it in at least two other projects at large companies. (2) Intentional or unintentional, this has the potential to crash debuggers and entire IDEs. It's simply not a safe bug to have in a Object-level function like ToString(), especially when that implementation cannot be changed by the developer.

[zaneclaes]

To be clear: I'm not arguing that protobufs should support bidirectional relationships overall. I understand why this would be undesirable. But the current implementation appears to serve no good purpose. Why does ToString() need to serialize into JSON? Isn't the whole point of protobufs to move AWAY from JSON? What if I don't want the serialized version of my messages to print the ENTIRE object?

All I'm saying is that this is not a good implementation of ToString(). If it were, say, Inspect() or ToJSON(), I'd be in support of it.

[xfxyjwf]

@zaneclaes As I said, what you are doing is not supported. When you intentionally add cycles into the class structure, the behavior of protobuf runtime should be undefined. If we can catch the error early without adding too much complexity, that's something we should do, but I don't think we want to add complexity or API to let you get around with it.

[zaneclaes]

I understand what you're saying. It's entirely reasonable to say that "cycles aren't supported." But I don't think you're addressing my point, @xfxyjwf , so let me try to explain myself better in technical terms, with a proposed solution (and an offer to contribute the necessary code):

The protobuf-generated code for ToString() is an invocation of JsonFormatter.ToDiagnosticString. This method is very well named: the ToString() method is frequently used in diagnostics. So let me make a simple assertion, and you can tell me if you disagree:

Good code should not ever crash diagnostic tools.

I think that's a pretty reasonable statement, don't you? I mean, crashing the IDE or inspector is a Very Bad Thing, regardless of if the end-developer was doing something unsupported.

And to reiterate, I've already proposed a very very simple fix in the original report: make it pluggable (or configurable). The JsonFormatter class already uses:

private static readonly JsonFormatter diagnosticFormatter = new JsonFormatter(JsonFormatter.Settings.Default);

to accomplish this serialization. Why not let the developer replace that instance with their own? Or change the configuration on the instance in some way? Why force the developer to use a default implementation of something which seems clearly designed to be configured anyways?

My proposal is to add the interface (which JsonFormatter implements):

public interface IDiagnosticFormatter {
  string ToDiagnosticString(IMessage message);
}

And the class:

public sealed class Diagnostics {
  public static IDiagnosticFormatter formatter = new JsonFormatter(JsonFormatter.Settings.Default);
}

And then simply allow the ToString() code generator to call Diagnostics.formatter.ToDiagnosticString(this);

Of course, we'd want to be a bit more defensive in the final implementation. In any case, thanks for reading this far ;)

[xfxyjwf]

@zaneclaes Please understand that we are very cautious about API changes. I do not think the problem you are having (code throws exceptions because of incorrect protobuf usage) is worth fixing with an API change. Users facing this problem should just fix their code. If it is causing IDE/inspector to crash, I think it's the IDE/inspector's problem because they should be able to work with any kind of code. If we are to introduce a feature that lets users customize ToString(), it should be for a better use case.