ObjectDisposedException and argument verification, esp. in crypto libraries

[GeeLaw]

News has it that PQC is being integrated with .NET 10. I was excited and came here to read the code. Here's some curious finding/question/suggestion.

In many cases the check for disposed object (ThrowIfDisposed();) is done after all other argument verifications. This is the case for all methods in System/Security/Cryptography/SlhDsa.cs. I also found System/Security/Cryptography/CompositeMLDsa.cs, where ExportCompositeMLDsaPublicKey checks for disposed object before arguments. In SlhDsa.VerifyData, object disposal is checked between two argument checks. It appears that verification for arguments themselves is done roughly in the order of parameter position.

Is there official guidance on the order of checking for disposed object and verifying arguments? I'm eager to learn more about it. Yet without understanding the reasoning, for high-stake libraries such as crypto, checking for disposed object after verifying other arguments, especially if the verification of those arguments depends on the ex-object (state before disposal), keeps me up at night. (A perfunctory search suggests that argument verification might depend on the algorithm specifications, but not non-public state.)

If there isn't a known guidance, I would argue for checking object disposal as the first thing done in public instance methods. This ensures no state-dependent logic is executed if object has been disposed, and I think that's generally good hygiene. In addition, the receiver (instance) is the implicit first argument, so if argument verification is done roughly in the order of parameter position, then the validity of this should be the first thing to check.

[Clockwork-Muse]

An IDisposable object isn't automatically collected, or anything like that, so this will always be valid; if you're calling an instance method it hasn't been garbage collected yet, and you have a valid reference to the instance. There isn't an equivalent to the traditional use-after-free accessing unowned memory as in languages like C/C++ (that's part of the point of having a GC, after all).

Now, the contents of the object may be in some indeterminate state, but generally that only applies to the unmanaged resources that necessitated the IDisposable in the first place; you only really need to check for disposal if you would touch them.

In many cases the check for disposed object (ThrowIfDisposed();) is done after all other argument verifications.

If the checks are referencing ArgumentException or its subclasses, generally these are intended to crash the process; they (usually) represent (calling) programmer error, and there is no reasonable recovery available outside of correcting whatever bug is causing the problem. In such a case, no other interactions with protected resources are done, and the resources would be cleaned up with the rest of process memory on exit.

[GeeLaw]

I'm not sure where to start, because I have no idea when GC and C/C++ entered the picture. I feel my points aren't conveyed clearly, so let me try again in the next top-level reply.

[GeeLaw]

My default mental model for a disposable object is as follows: If the object has been disposed, then the only information about this object, for users of this object to know, is that the object has been disposed; Dispose is idempotent; including further information only for debugging purposes is fine.

Whether this is a good model is subject to discussion. Assuming this model is the right one, let's check it with commit 89c8f6a112d37d2ea8b77821e56d170a1bccdc5a file SlhDsa.cs line 130, reformatted:

public void SignData(ReadOnlySpan<byte> data, Span<byte> destination, ReadOnlySpan<byte> context = default)
{
  int signatureSizeInBytes = Algorithm.SignatureSizeInBytes;
  if (destination.Length != signatureSizeInBytes)
  {
    throw new ArgumentException(
      SR.Format(SR.Argument_DestinationImprecise, signatureSizeInBytes),
      nameof(destination));
  }
  if (context.Length > MaxContextLength)
  {
    throw new ArgumentOutOfRangeException(
      nameof(context),
      context.Length,
      SR.Argument_SignatureContextTooLong255);
  }
  ThrowIfDisposed();
  SignDataCore(data, context, destination);
}

Suppose an SlhDsa instance is already disposed, then calling this method will reveal information about this.Algorithm.SignatureSizeInBytes (because one can detect when an ArgumentException is thrown), which is some information a user need not know, because it's about "what algorithm the object was using prior to disposal".

To avoid revealing such information, one can call ThrowIfDisposed as the first thing in the public methods.

Of course, this particular, simple example doesn't appear harmful, because the cryptographic algorithm being used is public information. However, at the vastness of .NET libraries, it's not easy to verify that fields accessed prior to various scattered ThrowIfDisposed()s are really OK to access/use/reveal information about in case the object has been disposed.

---

The above is just my model, and since what I observed in the official .NET libraries are somewhat different, it indicates

• either my model is incorrect (e.g., maybe too strict, or incompatible with other requirements), in which case I'd like to learn about whether there're, and what are, some official guidelines in checking for disposal and verifying arguments [in this case, a possible answer could be "there's no such guidelines and they can be verified in arbitrary order in general"],
• or my model is a good one and object disposal check and argument verification should be made in a consistent order, with disposal check first.

Hence the dual nature of question and suggestion.

---

For clarification, to respond to the previous top-level comment:

Now, the contents of the object may be in some indeterminate state

The object should be in a determinate state, the cannot-be-used state.

you only really need to check for disposal if you would touch them.

Just because something can be done does not mean it's a good idea to do it. Suppose an object X owns disposable resources D as well as resources/data N not subject to disposal. My default mental model forbids accessing N after X is disposed, regardless of whether D is being / will be / might be accessed in a method or a series of operations. This is not a CLR or GC restriction, not a restriction of D, not a restriction of N, (so yes, it's not needed as far as CLR, GC, D, N are concerned) but a restriction put into the design of X.

In many cases the check for disposed object (ThrowIfDisposed();) is done after all other argument verifications.

If the checks are referencing ArgumentException or its subclasses, generally these are intended to crash the process; they (usually) represent (calling) programmer error, and there is no reasonable recovery available outside of correcting whatever bug is causing the problem.

I agree that ArgumentException indicates programming error and the bug should be corrected by the programmer. My point is I regard ObjectDisposedException as the same kind of programming error and bug, because it is logically a kind of argument exception, with the argument being the receiver object pointed to by this in the example. This is another reason to check it first --- because this is the implicit first parameter/argument to the methods, and other arguments are checked roughly in parameter position order, so for consistency.

An IDisposable object isn't automatically collected, or anything like that, so this will always be valid; if you're calling an instance method it hasn't been garbage collected yet, and you have a valid reference to the instance. There isn't an equivalent to the traditional use-after-free accessing unowned memory as in languages like C/C++ (that's part of the point of having a GC, after all).

Not applicable to the intended meaning of my original post, but I do thank you for pointing out a natural language usage issue --- my original post:

the validity of this should be the first thing to check.

should be changed to:

the non-disposed state of the object referenced by this should be the first thing to verify.

[bartonjs]

We decided to do argument validation first for consistency.

public byte[] SignData(byte[] data, byte[] context) wants to defer to SignData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> context), but do null-checks. If Dispose is first, then it looks like

public byte[] SignData(byte[] data, byte[]? context = null)
{
    ThrowIfDisposed();
    ArgumentNullException.ThrowIfNull(data);
    return SignData(new ReadOnlySpan<byte>(data), new ReadOnlySpan<byte>(context));
}

public byte[] SignData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> context)
{
    if (context.Length > 255) throw ...;

    ThrowIfDisposed();
    ...
}

So that path makes the array path go through two ThrowIfDisposed if we strongly care about "Dispose first". But it's really easy to end up modelling it as

public byte[] SignData(byte[] data, byte[]? context = null) => SignData(ROSpanOrThrow(data), context);

public byte[] SignData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> context = default)
{
    ThrowIfDisposed();

    if (context.Length > 255) throw ...;
    ...
}

Now, let's assume that we decided not to ship SignData(byte[], byte[]), but someone wanted to define it as an extension method. They have no access to the disposed state. They could randomly call some function with a side-effect of throwing when disposed, or they can ignore it and just do argument validation.

public static byte[] SignData(this MLDsa mldsa, byte[] data, byte[]? context = null)
{
    ArgumentNullException.ThrowIfNull(mldsa);
    ArgumentNullException.ThrowIfNull(data);
    return mldsa.SignData(new ReadOnlySpan<byte>(data), context);
}

In these latter two cases, the null check for an array happens before the dispose, but the check for context.Length happens after. It's inconsistent, and therefore bad.

The extension method approach also aligns with wrapper instances, where the true disposal state lies in the wrapped object. Wrapper types can independently track disposal, but that means they're just tracking it to promote the ODE above the AOORE. An annoying amount of work for a thing that, in general, shouldn't ever matter.

So what we decided on was:

• Argument validation that can be done without checking disposed
• Disposed state check
• Remaining argument validation (hopefully none)
• Work

If we queried parameter set information repeatedly, it would depend on the disposed state. But since any instance of MLDsa can only ever have one parameter set, we capture it at construction time and don't let it go. That moves it to "before disposed". Sort of stateful, sort of stateless.

We also decided in some cases to skip disposed checks when cascading from one method to another. But, again, from the model of "what could an extension method do?"

public static byte[] SignData(this MLDsa mldsa, ReadOnlySpan<byte> data, ReadOnlySpan<byte> context = default)
{
    ArgumentNullException.ThrowIfNull(mldsa);
    if (context.Length > 255) throw ...;

    byte[] ret = new byte[mldsa.Algorithm.SignatureSizeInBytes];
    mldsa.SignData(data, ret, context);
    return ret;
}

That array gets allocated if we don't reject context, but mldsa could be disposed. Oh, well.

---

At the end of the day, the best model, therefore, is "what could an extension method do?" Unless we decide to pattern attach public void ThrowIfDisposed(); to all IDisposable, the only real answer is "argument validation first". And since API Review didn't even let protected void ThrowIfDisposed(); through, the public version seems unlikely.

[GeeLaw]

That's a really interesting perspective. Learned something new today. Thanks!