Add check for null db parameter value

[HoberMellow]

Code inside ConvertValueToNumericIfEnumr throws null reference exception on null parameter value. I've tried to find confirmation that this value cannot be null but with no luck. In EF 6 sources null in parameter value treated equally to DbNull mostly and there are many null checks. Also i want to note that calling EF code has checks for null for two out of three parameters (DbProviderServices.SetDbParameterValue checks null for parameter and parameterType but not for value - https://github.com/dotnet/ef6/blob/master/src/EntityFramework/Core/Common/DbProviderServices.cs#L521). So i believe null is a valid value so this check is necessary here.

Also it will fix this issue: https://github.com/npgsql/EntityFramework6.Npgsql/issues/126

[julealgon]

@roji is there any particular reason why this hasn't been merged yet? This issue has been going for several years now and is impacting one of our integrations as well.

[roji]

@julealgon the change does look OK to me, but this project has been in "archive mode" in quite a while, and I simply am not familiar with EF6. Ideally there would be a repro (and even a regression test) that confirm that this is a bug etc - #126 has a sort of repro, but it relies on a 3rd-party component and things aren't very clear.

Given the state of things, if someone can do the legwork to understand what's going on and show how the bug can be reproduced with EF6, I can merge this and release a new version... But otherwise I just don't have the time to do it myself.

[replaysMike]

This is a pretty simple null-ref bug that doesn't require EF6 knowledge. GetType() is being called potentially on a null value within the parameter being passed. I agree this is evidently reproducible by a third-party package, but the fix is fairly obvious by looking at the commit.

[roji]

The question is whether it makes sense for the parameter to come in with a null Value in the first place; otherwise returning early like this may move the bug to somewhere else.

Can nobody provide a repro for this without the 3rd-party?

[julealgon]

@roji

@julealgon the change does look OK to me, but this project has been in "archive mode" in quite a while, and I simply am not familiar with EF6.

Fair enough. Even though I don't think this requires deep EF6 knowledge to understand (like mike alluded to above), is there anybody on the team here that could review and merge this besides you?

This specific bug is preventing us from migrating away from fairly old versions of Npgsql in a 120+ project solution. Our stack is not yet at the point where we can ditch it in favor of EFCore (even though we do intend to have that dealt with in the future) so being able to move forward would provide significant benefits for us. This specific dependency on Npgsql actually interferes with other libraries which also rely on Npgsql to work on our side, like Rebus. Upgrading one but not the other results in all sorts of MissingMethodExceptions and just flat out doesn't work.

Could you please help us out on this one? What exactly do you need for us to move forward with the merge? We have a few stacktraces that we could partially share. Does this help?

System.NullReferenceException: Object reference not set to an instance of an object. at System.Object.GetType() at Npgsql.NpgsqlServices.ConvertValueToNumericIfEnum(DbParameter parameter) at System.Data.Entity.Core.EntityClient.Internal.EntityCommandDefinition.SyncParameterProperties(EntityParameter entityParameter, DbParameter storeParameter, DbProviderServices storeProviderServices) at System.Data.Entity.Core.EntityClient.Internal.EntityCommandDefinition.PrepareEntityCommandBeforeExecution(EntityCommand entityCommand) at System.Data.Entity.Core.EntityClient.Internal.EntityCommandDefinition.ExecuteStoreCommands(EntityCommand entityCommand, CommandBehavior behavior) at System.Data.Entity.Core.Objects.Internal.ObjectQueryExecutionPlan.Execute[TResultType](ObjectContext context, ObjectParameterCollection parameterValues) at System.Data.Entity.Core.Objects.ObjectContext.ExecuteInTransaction[T](Func1 func, IDbExecutionStrategy executionStrategy, Boolean startLocalTransaction, Boolean releaseConnectionOnSuccess) at System.Data.Entity.Core.Objects.ObjectQuery1.<>c__DisplayClass41_0.b0() at System.Data.Entity.Core.Objects.ObjectQuery1.GetResults(Nullable1 forMergeOption) at System.Data.Entity.Core.Objects.ObjectQuery`1.<System.Collections.Generic.IEnumerable.GetEnumerator>b31_0() at System.Data.Entity.Internal.LazyEnumerator1.MoveNext() at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable1 source)

We have already confirmed that the change in this PR fixes the issue completely on our side.

[replaysMike]

True, assuming that throwing a null-ref exception is the current acceptable way of preventing it. We have been running with this patch since the PR was created. I'm only aware of this on the DynamicFilters package, and this issue isn't present on the SqlServer provider.

[roji]

@julealgon are you also using DynamicFilters? If not, can you reproduce this in a simple console app or similar?

[zoranmihajlovski-allocate]

Same here using DynamicFilters with Postgre provider, we are stuck with v3.1.1 and we cant upgrade because of this issue. Do you have any workaround?

[julealgon]

@roji

@julealgon are you also using DynamicFilters? If not, can you reproduce this in a simple console app or similar?

I'm in the same team as @replaysMike above, so yes. We tried creating a minimal repro internally without relying on DynamicFilters explicitly (using a custom command interceptor) but due to time constraints were unable to for now.

Still, I find it unfortunate that you are unwilling to accept this as a problem considering the value in question is nullable in the model: if it was not supposed to be nullable, then it's constructor/property setter should guarantee against such. The fact that a consumer (no matter which one) can interface with your code and cause it to throw a null reference exception internally is very clearly a bug on your library, and not on the caller side.

There are only 2 possible outcomes: either the value is missing a guard clause (and thus should throw an ArgumentNullException when attempting to construct with or assign a null value), or it should be handled appropriately when null. I'm confident the latter is the case here, since the former would cause a breaking change on the base abstractions which would in turn break all other providers.

With the above in mind, I'd sincerely ask you to reconsider this one more time, even without a direct repro not relying on DynamicFilters. As mentioned earlier, other providers, like the SQL Server one, do not exhibit this issue (obviously, that doesn't necessarily mean you have a problem per se, it's just another indicator).

While we do have a workaround now (read below), it is still just that: a workaround. We'd rather see the actual fix in place and be able to seamlessly use it.

---

@zoranmihajlovski-allocate

Same here using DynamicFilters with Postgre provider, we are stuck with v3.1.1 and we cant upgrade because of this issue. Do you have any workaround?

We are actually testing a workaround now, which relies on the Harmony2 code injection library and module initializers. We have introduced the fix in this PR as a method prefix using Harmony and then applied the patch in a module initializer in the assembly where our db contexts are (in our case, a good enough centralized point for doing the patch).

This appears to be working very well against the latest version of Npgsql, so it's probably the approach we'll take since @roji doesn't want to merge this and we can't dedicate as much time to the repro as we want. It's not the best solution, but it beats having to fork and maintain the code on our side in our team's opinion.

[roji]

First, before you go ahead with runtime patching in a module initializer, I didn't say I don't want to merge this - I asked for help from the community to verify the bug first. That would also have enabled us to add a regression test.

The fact that a consumer (no matter which one) can interface with your code and cause it to throw a null reference exception internally is very clearly a bug on your library, and not on the caller side.

That's not true. It's possible that method simply shouldn't accept nulls, and the bug is in DynamicFilters. In that case the method would indeed need to throw ArgumentNotException (and not NullReferenceException).

But regardless, I looked at it and it does look safe OK to simply return for null. I'll merge this and release a patch version.

[roji]

Version 6.4.2 is out and available on nuget.org, with this patch.

[julealgon]

Thanks for you support @roji . While the patching approach worked on our end, it was still a very hacky solution so I'm glad we'll be able to just reference the latest version now and be done with it.

[AaronAllBright]

Hey @roji thank you for your help as well. I'm sad to report that the 6.4.2 update is failing strong name signature validation.

[roji]

@AaronAllBright my bad, I built the library incorrectly... I've released 6.4.3 which should pass strong name validation - can you please confirm?

[AaronAllBright]

Looks good now! Thanks again @roji

[zoranmihajlovski-allocate]

All good on our side as well, thanks alot @roji, you are lifesaver :)