Aspnetcore: Inifinite loop during model binding when using [ModelBinder] attribute

[Costo]

Is this a Bug or Feature request?:

Bug

Steps to reproduce (preferably a link to a GitHub repo with a repro project):

Link to repro: https://github.com/Costo/aspnetcore-binding-bug

• Run project with dotnet run
• Go to https://localhost:5001
• Submit the Form

Expected result:

• Form is submitted and server responds with 200 OK

Actual result:

• Server never responds. If you attach a debugger and add a breakpoint to NumberModelBinder.BindModelAsync, you can see that the program is in an inifinite loop (see screenshot below).

Description of the problem:

The problem occurs when using a custom Model Binder for a decimal property in this model:

public class TestModel
{
    public TestInnerModel[] InnerModels { get; set; } = new TestInnerModel[0];
}

public class TestInnerModel
{
    public TestInnerModel()
    {
    }

    [ModelBinder(BinderType = typeof(NumberModelBinder))]
    public decimal Rate { get; set; }
}

During a POST, it seems that model binding is stuck in an infinite loop in CollectionModelBinder

The code of the NumberModelBinder class is actually the code of DecimalModelBinder, with internal logging code stripped out, so nothing out of the ordinary.

If the [ModelBinder] attribute is removed, the application works fine.

Version of Microsoft.AspNetCore.Mvc or Microsoft.AspNetCore.App or Microsoft.AspNetCore.All:

netcoreapp2.1 or netcoreapp2.2 with SDK 2.2.100-preview3-009430

[mkArtakMSFT]

Thanks for contacting us, @Costo.
This seems like an issue with your code, rather than ASP.NET Core framework.
While we do our best to look through all the issues filed here, to get a faster response we suggest posting your questions to StackOverflow using the asp.net-core-mvc tag.

[mkArtakMSFT]

@dougbu, can you please see whether something obvious is going on here? Thanks!

[Costo]

Thank you for your response @mkArtakMSFT.

I simplified the repro even further to make make it not about our code. The custom binder now just calls the DecimalNumberBinder like this:

public class NumberModelBinder : IModelBinder
{
    private readonly NumberStyles _supportedStyles = NumberStyles.Float | NumberStyles.AllowThousands;
    private DecimalModelBinder _innerBinder;

    public NumberModelBinder(ILoggerFactory loggerFactory)
    {
        _innerBinder = new DecimalModelBinder(_supportedStyles, loggerFactory);
    }

    public Task BindModelAsync(ModelBindingContext bindingContext)
    {
        return _innerBinder.BindModelAsync(bindingContext);
    }
}

The custom model binder works well if used on top level model properties. The problem occurs only if the custom model binder is used on properties of an array of "child" models.

I'll try to post this on StackOverflow as well.

[Costo]

Here's the Stack Overflow question: https://stackoverflow.com/questions/53303414/inifinite-loop-during-model-binding-when-using-modelbinder-attribute

[dougbu]

@Costo The problem is you're not informing the model binding system that the binder uses value providers. The ComplexTypeModelBinder always believes data is available for the next TestInnerModel instance and the outermost binder (CollectionModelBinder) keeps going -- forever. To fix this,
``` c#
[MyModelBinder]
public decimal Rate { get; set; }

private class MyModelBinderAttribute : ModelBinderAttribute
{
public MyModelBinderAttribute()
: base(typeof(NumberModelBinder))
{
BindingSource = BindingSource.Form;
}
}
```

---

Put another way, the BindingSource.Custom default [ModelBinder] uses isn't correct in this scenario. Fortunately custom model binders on properties of POCO types in containers should be one of the very few cases where this matters.

[Costo]

It works! 🎉 🎉 🎉
Thank you @dougbu for this solution.

Now, originally our custom model binder was used to accept both the dot (.) and the comma (,) as a decimal separator in French culture. Is there another way to achieve this other than with a custom model binder? We tried the TypeConverter approach suggested in the docs but it didn't seem to work.

[dougbu]

@Costo I suggest inserting another provider earlier in MvcOptions.ModelBinderProviders than FloatingPointTypeModelBinderProvider. That provider should handle only decimal models and instantiate the DecimalModelBinder with your preferred NumberStyles.

[Costo]

Ok, I'll try this approach. Thank you for your help. Closing this.

[dougbu]

Reopening for discussion of the more general issue: Model binding getting into an infinite loop is never good and the root cause isn't obvious.

[Costo]

@matomato13 mentioned to me that the loop is technically not infinite but will continue until reaching int.MaxValue. I never waited that long 😄

[dougbu]

This issue occurs due to a number of sometimes-mistaken assumptions in MVC:

• Greedy binding sources are defined to mean the source is bound in a single operation. And, because the predefined non-greedy sources all use value providers, ComplexTypeModelBinder assumes the value providers don't matter for greedy sources.
  
  
  
  • The binding source may be ignored when choosing binders, custom binders may or may not use value providers, and users may define custom greedy binding sources.
  
  
  • ComplexTypeModelBinder also assumes that greedy sources (for properties) are associated with binders that _cannot_ fail. Well, it's assuming recursion into a greedy source is safe because further recursion won't occur. But, the impact is potentially misleading higher-level binders (like the CollectionModelBinder in this issue).
  
  
• The model binding system assumes binders specified using [ModelBinder(BinderType = typeof(...))] are greedy.
  
  
  
  • The system actually has no idea where the custom binder will find data. Might also not be a greedy binder, because binders may be created at any point, the custom binder might recurse.
  
  

Now that model binders are chosen in an initial phase, not per-request, the most thorough way to fix this would be to get more information from the chosen binder. WillRecurse and UsesValueProviders would be separate properties of a model binder. IsGreedy and IsFrromRequest would remain primarily as hints to API Explorer. But, this change may be too big a break for 3.0.

Other, poorer, possibilities

1. Make BindingSource.Custom non-greedy. This could lead to under-binding if the model binder actually uses value providers. And, it could lead to infinite recursion if the model binder will recurse.
2. Change [ModelBinder] to leave its BindingSource property null when BinderType is specified. But, ComplexTypeModelBinder would then, perhaps incorrectly, assume the value providers matter. Infinite recursion would also be possible.

---

Regardless of the above fixes, ComplexTypeModelBinder should fail when a model has only greedy sources and their bindings all fail. That alone would fix this scenario but leave the poor assumptions.

[dougbu]

@pranavkm @mkArtakMSFT @rynowak @Eilon thoughts on the different options described above and whether it's worth breaking i.e. extending IModelBinder in 3.0? (Getting more information from the binders is only a complete fix if we get it from all binders.)

[Eilon]

I know very little about this area of model binding, but @rynowak knows a fair bit.

Is a breaking change worth it? I don't know, but it would have to be extremely compelling to warrant breaking a major interface.

[rynowak]

@dougbu and I discussed this offline. We think there's a straightforward set of improvements here.

The main concern is that it's altogether too easy to have an infinite loop, and altogether too hard to understand what to do to prevent it. This is primarily because the default for [ModelBinder(...)] is to set BindingSource.Custom which is greedy and can lead to infinite loops without being used carefully.

We propose to change the following for [ModelBinder(...)]

• Add a constructor that also accepts a BindingSource to make it easier to specify
• Change the default BindingSource to BindingSource.ModelBinding.

The rationale for the change is that BindingSource.ModelBinding is the correct case for a model binder that looks at value providers. It's really common that a custom model binder extends ComplexTypeModelBinder and usually user code is looking at value providers. You only see differences in behaviour when collections and recursion is involved, and most of those cases today where someone relies on BindingSource.Custom's greed would be bugs (like this one). We think this will prevent pain.

---

We also propose adding a new property to BindingSource called UsesValueProvider or similar. We want to be able to tell the difference between model binders that aren't greedy - but also don't use value providers. This will help us avoid similar situations to this bug. As an example, we would change BindingSource.Header be non-greedy but UsesValueProvider == true - you could reproduce this same issue with [FromHeader] and this new setting will address that.

---

Regardless of the above fixes, ComplexTypeModelBinder should fail when a model has only greedy sources and their bindings all fail. That alone would fix this scenario but leave the poor assumptions.

We discussed in a very shallow level of detail and it mostly makes sense as a systemic fix for some of these problems. @dougbu will file a separate issue for this to be considered.

[effyteva]

Waiting for this fix as well,
I would assume bugs should be released on a 2.2.1/2.3 branch, and not only 3.0, as the 3.0 branch doesn't support .NET Framework anymore...

[dougbu]

Discussed offline w/ @rynowak and @mkArtakMSFT:

• do not change BindingSource.Header other than to add IsFromValueProviders == false
  
  
  
  • slight typo in @rynowak's comments above and slightly more consistent name for the new property
  
  
• do not change ComplexTypeModelBinder's assumption that model binding which do not use value provides always succeed i.e. continue to ignore the failure of model binders once ComplexTypeModelBinder decides to proceed
  
  
  
  • Cases where this matters (outside the original scenario) are rather arcane. E.g. failure cases arise only when _no_ properties of a complex type use value providers.
  
  
• change ComplexTypeModelBinder to assume binding will succeed based on IsFromValueProviders instead of greediness

[dougbu]

Oops: Leave BindingSource and ComplexTypeModelBinder alone. That addition / enhancement isn't solid enough at this time to avoid concerns about causing additional model binding issues.

---

Minor but may be helpful to add doc comments in the code users see when using ModelMetadata extensibility to override the binder for a type. These comments would suggest updating the BindingSource as well i.e. to avoid similar confusions to using [ModelBinder].

[rynowak]

And we want to change ModelBinderAttribute as described above so that the default is more typical.

[dougbu]

Punting to Preview3 because #4802 is more urgent

[dougbu]

After doing #7052 fix (3e0c75187c), minimal changes related that are specific to this issue were merged in 14b7184c09