Aspnetcore: IFormFile bind error when using in sub collection

[megafetis]

Describe the bug

When i trying to post form with sublist of images models with IFormFile properties, the program starts to hang!
see the screen

To Reproduce

Steps to reproduce the behavior:

1. Using this version of ASP.NET Core '2.2'
2. Run this code

```// Model for edit form
public class CategoryCreateModel
{
public string Id { get; set; }
public string Name { get; set; }
public int Priority { get; set; }
public string Abbr { get; set; }
public string Description { get; set; }
public string LinkName { get; set; }
public string ParentId { get; set; }
// here error binding
public IList Images { get; set; }
}

//uploaded images sub collection
public class AppImageEditModel
{
public string Id { get; set; }
public int Priority { get; set; }
public string Name { get; set; }
public string ImageUrl { get; set; }
// if class contains IFormFile prop, hapends binding bug
public IFormFile AppImageFile { get; set; }
public AppImageType ImageType { get; set; }
public bool IsMain { get; set; }
}

// in controller action:
[HttpPost]
public async Task Create(CategoryCreateModel model)
{
if (ModelState.IsValid)
{
await Mediator.Send(model);
return RedirectToAction("Index");
}
return View(model);
}

```
No errors thrown, but frease executing:
operative memory encreases from 100 megabytes to 3-4 gigabytes!!!

Screenshots when I submit form:

Expected behavior

The execution is not come to action method and hangs

Binding in razor inside the form:

<form asp-action="Create" enctype="multipart/form-data">
...
<input type="text" name="Images[0].Name" id="Images_0__Text" />
<input type="file" name="Images[0].AppImageFile" id="Images_0__AppImageFile" />

<input type="text" name="Images[1].Name" id="Images_1__Text" />
<input type="file" name="Images[1].AppImageFile" id="Images_1__AppImageFile"/>

Additional context

if you delete the property public IFormFile AppImageFile from class AppImageEditModel, the program will work well!

[mkArtakMSFT]

Thanks for contacting us, @megafetis.
This is actually a dupe of https://github.com/aspnet/Mvc/issues/8782, which for the fix is tracked as part of https://github.com/aspnet/AspNetCore/issues/4939

[avanveelen]

Last week I encountered the same issue, but also found a work around for it.
So, for people facing the same issue and who can not wait for a fix:

When you set the index as a hidden value in the form post, the dotnet Core model binding does not not fall into an infinite loop.
Like this:

<input type="hidden" name="Images.Index" value="0" />
<input type="text" name="Images[0].Name" />

<input type="hidden" name="Images.Index" value="1" />
<input type="text" name="Images[1].Name" />

I have made a small reproduction of the bug and included the work around.

[megafetis]

Thank you! Cool idea

[aminanan]

I would like to point out that there is no issue with previous versions of ASP.net Core. When we upgraded to ASP.NET Core '2.2' we got an OutOfMemoryException due to this issue. Now we reverted to ASP.NET Core 2.1.7 waiting for a fix.

[mkArtakMSFT]

Thanks for bringing this up, @hakimon.
@dougbu, can you please investigate this? Thanks.

[dougbu]

This sounds like another variant of #4939 and #6616. But, it has nothing to do with [BindProperty] or [ModelBinder].

Ugly workaround would be something like
``` c#
public class CategoryCreateModel
{
public string Id { get; set; }
public string Name { get; set; }
public int Priority { get; set; }
public string Abbr { get; set; }
public string Description { get; set; }
public string LinkName { get; set; }
public string ParentId { get; set; }

    // Following two collections should have the same number of items.
    public IList<IFormFile> Images { get; set; }
    public IList<AppImageEditModel> ImageMetadata { get; set; }
}

// uploaded images sub collection
public class AppImageEditModel
{
    public string Id { get; set; }
    public int Priority { get; set; }
    public string Name { get; set; }
    public string ImageUrl { get; set; }
    public AppImageType ImageType { get; set; }
    public bool IsMain { get; set; }
}

```

[dougbu]

@danroth27 @mkArtakMSFT @rynowak if possible, we should try to get this into Preview2 and consider it for a near-term 2.2.x patch

[colotiline]

Last week I encountered the same issue, but also found a work around for it.
So, for people facing the same issue and who can not wait for a fix:

When you set the index as a hidden value in the form post, the dotnet Core model binding does not not fall into an infinite loop.
Like this:

<input type="hidden" name="Images.Index" value="0" />
<input type="text" name="Images[0].Name" />

<input type="hidden" name="Images.Index" value="1" />
<input type="text" name="Images[1].Name" />

I have made a small reproduction of the bug and included the work around.

Workaround works.

But it's unacceptable in production. Someone can crash the app sending a valid request.

[WahidBitar]

another solution different than @avanveelen workaround

you can just wrap the IFormFile in another class and it will work

    public class Company
    {
        public IList<Person> Employees { get; set; } = new List<Person>();
    }

    public class Person
    {
        public FormFileWrapper IdImage { get; set; }
    }

    public class FormFileWrapper
    {
        public IFormFile File { get; set; }
    }

then you don have to add the index in your html

<form action="http://localhost:5000/api/Values" method="post" enctype="multipart/form-data">
    <p><input type="file" name="Employees[0].IdImage.File"></p>
    <p><input type="file" name="Employees[1].IdImage.File"></p>
    <p><button type="submit">Submit</button></p>
</form>

[Leftyx]

@dougbu The issue is still present for recursive models.
This class:

public class MenuItemModel
{
    public IList<MenuItemModel> SubItems { get; set; }
}

would trigger the same issue with the committed ComplexTypeModelBinder.

[dougbu]

5bbf7109a5

[Plasma]

5bbf710

Hi @dougbu is there a plan to merge this into the 2.2x branch also and do a patch update?

At the moment 2.2 is dangerous because its easily crashed.

Thanks

[dougbu]

@Plasma we do not have plans to backport this fix to the release branches because Model Binding changes have a history of causing different problems. We have not had reports of new issues in the 3.0 releases, but…

/cc @mkArtakMSFT

[Plasma]

Ironically the model change caused this problem :P - thank you for your efforts, but I do want to note the latest 2.2x release is unstable because of this, and has come as a bit of a surprise based on all the other performance/stability work gone into .NET Core to date to be left in this state.

[akw123]

another solution different than @avanveelen workaround

you can just wrap the IFormFile in another class and it will work

    public class Company
    {
        public IList<Person> Employees { get; set; } = new List<Person>();
    }

    public class Person
    {
        public FormFileWrapper IdImage { get; set; }
    }

    public class FormFileWrapper
    {
        public IFormFile File { get; set; }
    }

then you don have to add the index in your html

<form action="http://localhost:5000/api/Values" method="post" enctype="multipart/form-data">
    <p><input type="file" name="Employees[0].IdImage.File"></p>
    <p><input type="file" name="Employees[1].IdImage.File"></p>
    <p><button type="submit">Submit</button></p>
</form>

Thanks @WahidBitar . Tested, workaround works, though tbh not the most glamorous way of doing things. This would work for us now. @dougbu looking forward to the 3.0 official release

[threax]

Since all the answers here are a bit of a hack I thought I would add my 2 cents.

In my case I was not really able to modify my input model, so instead I instructed the c;lass with the file to [BindNever] the IFormFile property.

public class InputWithFile
{
    public bool OtherProperty { get; set; }

    [BindNever]
    public IFormFile UploadFile { get; set; }
}

Then in the wrapper class I wrote a function that manually pulls the files out of the form.

public class InputWithFileUploadList
{
    public int SomeNumber { get; set; }

    public List<InputWithFile> InputWithFiles { get; set; }

    public void HACK_FixUploadFiles(HttpContext httpContext)
    {
        if (InputWithFiles != null)
        {
            int count = 0;
            foreach (var req in InputWithFiles)
            {
                var propName = $"inputWithFileUploadList[{count++}].uploadFile";
                req.UploadFile = httpContext.Request.Form.Files.Where(i => propName.Equals(i.Name, StringComparison.InvariantCultureIgnoreCase)).FirstOrDefault();
            }
        }
    }
}

You will have to call HACK_FixUploadFiles in your controller before using the input model. In my case this was only in 1 spot if you have multiple places you are using this input model this may be hard to maintain, but this works well if you can't modify the structure of your input models.

You will also likely have to change the line var propName = $"inputWithFileUploadList[{count++}].uploadFile"; to whatever your file input is actually called.

I tried various permutations using custom model binders and other techniques, but I kept getting the infinite loop using any kind of model binder. This will bypass all of that completely. It can be taken out once 3.0 ships.

[ericsampson]

@dougbu it seems like this deserves a backport for 2.2.x given the impact?

[dougbu]

it seems like this deserves a backport for 2.2.x given the impact?

Repeating a previous answer:

we do not have plans to backport this fix to the release branches because Model Binding changes have a history of causing different problems. We have not had reports of new issues in the 3.0 releases, but…

/cc @danroth27 and @mkArtakMSFT

[ericsampson]

I saw that answer, but it seems really unsatisfying; just because it's fixed in 3.0 it won't help the people who are going to be on 2.2 for the foreseeable future. Are you really going to leave a known impactful (potentially OOM triggering) bug unfixed in 2.2? cc @rynowak if you have any thoughts. Thanks all!

[mkArtakMSFT]

Hi everyone.
Given how much pain this is causing to the customers, we're going to reevaluate this decision. Please don't take this as a promise that we're going to fix this issue. Saying this is a risky area is an understatement.
For now, I'm reopening the issue before we decide what the next actions will be.

[mkArtakMSFT]

Marking this as blocked till we make a decision regarding whether we're fixing this or not.

[ericsampson]

Thanks for considering if/how to safely handle this @mkArtakMSFT and @dougbu, that's all we can ask for! :) Cheers

[mkArtakMSFT]

Moving this to 3.0-preview8 milestone to indicate the timeframe we'll be looking into this. There is still no decision regarding this being patched or not.

[willseward]

Just chiming in here because this just bit us, too. I would also consider this to be a vulnerability, since it has the implication of facilitating a DoS attack. I would assume that a backport of the mitigation should be considered heavily.

[mkArtakMSFT]

Hi folks.
We had a long conversation about this change and evaluating risks associated with potential fixes.
The conclusion is that we won't be able to patch this for the 2.2 release. Given that 3.0 will ship in just couple of months, we recommend customers switch to 3.0 instead, as the scenario is fundamentally fixed in it.

We understand that this is not ideal, but it's much better than shipping "some fix" and finding out that it now broke some other important scenario.

/cc @DamianEdwards, @danroth27

[bahmansh]

I have installed .net core 3.
Now I have no run time error, but iformfile will pass null.

https://github.com/dotnet/core/issues/3037

@mkArtakMSFT