Uh oh!
There was an error while loading. Please reload this page.
OLEDB: Fix for AV Exception on x86 architecture - #32150
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| ApplyParameterBindings(commandWithParameters, bindings.BindInfo); | ||
| if (bindings.BindInfo_x86 != null) | ||
| { | ||
| ApplyParameterBindings(commandWithParameters, bindings.BindInfo_x86); |
There was a problem hiding this comment.
You can make the ApplyParameterBindings to take bindings as an argument, and then do the switch for x86 inside the method. I believe there will be less duplicated code that way.
| #endif | ||
| #if (WIN32 && !ARCH_arm) | ||
| [StructLayoutAttribute(LayoutKind.Sequential, Pack = 2)] |
There was a problem hiding this comment.
Comment highlighting the difference in the x86 version may be useful
Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
| } | ||
| #endif | ||
| #if (WIN32 && !ARCH_arm) |
There was a problem hiding this comment.
I see the same ifdef on many other structs in this file. Do all of them need to be fixed?
There was a problem hiding this comment.
Either that or we shoukd remove the attributes/defines for them
There was a problem hiding this comment.
The other structs will need to be fixed as well. I can take those up in another PR. I vote for this particular fix to be merged as this has been reported by users and can be caught in the tests as well (while running on x86 platform). The tests are not catching the other cases, so I think there may be more tests needed to validate the changes to the other structs here.
There was a problem hiding this comment.
I am fine with doing just a baby step if we believe that this library is useful for real workloads on x86 without fixing all/most of these. What are the chances that customers hitting this crash will immediately hit the next once once this one is fixed? Do we have some more complex sample app that uses this library that we can use to validate that it can work for a real app on x86?
| { | ||
| internal sealed class Bindings | ||
| { | ||
| private static readonly bool s_runningOnX86 = RuntimeInformation.ProcessArchitecture == Architecture.X86; |
There was a problem hiding this comment.
The old defines referenced "ARCH_arm". (I assume that referred to 32 bit.) Do we expect this library to work on ARM/ARM64? If so will the right things happen?
There was a problem hiding this comment.
Yes, the right things will happen for ARM/ARM64. This is the condition that we are replicating from oledb.h from Windows SDK:
#if defined(_WIN64) || defined(_ARM_)
#include <pshpack8.h> // 8-byte structure packing
#else
#include <pshpack2.h> // 2-byte structure packing
#endif
| set | ||
| { | ||
| _bindInfo[_index].pwszName = value; | ||
| if (s_runningOnX86) |
There was a problem hiding this comment.
Ternary expression might be neater for these (?)
| } | ||
| #endif | ||
| #if (WIN32 && !ARCH_arm) |
There was a problem hiding this comment.
Either that or we shoukd remove the attributes/defines for them
| internal byte bPrecision; | ||
| internal byte bScale; | ||
| #if DEBUG |
There was a problem hiding this comment.
If you keep this I suggest "#if 0" since one would only need it in DEBUG if specifically looking at this problem. It's not a big deal but it does override a member that in theory other code could call.
There was a problem hiding this comment.
I did this to keep the style in the rest of this file. Otherwise I don't mind just removing the whole thing
| } | ||
| OleDbHResult hr = commandWithParameters.SetParameterInfo((IntPtr)bindInfo.Length, ordinals, bindInfo); | ||
| OleDbHResult hr; | ||
| fixed (tagDBPARAMBINDINFO* p = &bindInfo[0]) |
There was a problem hiding this comment.
@jkotas Would it be better to take the bindInfo here and check if we are running on x86 platform, and if we are, then create an array of tagDBPARAMBINDINFO_x86 and Marshal the tagDBPARAMBINDINFO to the array of x86 structs?
This would avoid all the if statements and multiple parameters accepting multiple structures based on the platform. I mean something like the following.
privateunsafevoidApplyParameterBindings(UnsafeNativeMethods.ICommandWithParameterscommandWithParameters,tagDBPARAMBINDINFO[]bindInfo){IntPtr[]ordinals=newIntPtr[bindInfo.Length];for(inti=0;i<ordinals.Length;++i){ordinals[i]=(IntPtr)(i+1);}OleDbHResulthr;if(s_runningOnX86){tagDBPARAMBINDINFO_x86[]bindInfo_x86=newtagDBPARAMBINDINFO_x86[bindInfo.Length];for(inti=0;i<bindInfo.Length;i++){
fixed (tagDBPARAMBINDINFO*p=&bindInfo[i]){bindInfo_x86[i]=(tagDBPARAMBINDINFO_x86)Marshal.PtrToStructure((IntPtr)p,typeof(tagDBPARAMBINDINFO_x86));}}
fixed (tagDBPARAMBINDINFO_x86*p=&bindInfo_x86[0]){hr=commandWithParameters.SetParameterInfo((IntPtr)bindInfo.Length,ordinals,(IntPtr)p);}}else{
fixed (tagDBPARAMBINDINFO*p=&bindInfo[0]){hr=commandWithParameters.SetParameterInfo((IntPtr)bindInfo.Length,ordinals,(IntPtr)p);}}if(hr<0){ProcessResults(hr);}}And ofcourse we change the signature of SetParameterInfo to
System.Data.OleDb.OleDbHResultSetParameterInfo([In]IntPtrcParams,[In,MarshalAs(UnmanagedType.LPArray)]IntPtr[]rgParamOrdinals,[In]IntPtrrgParamBindInfo);as @maryamariyan has already done?
There would be data duplication and manipulation for x86 scenario, but it does simplify the code greatly.
There was a problem hiding this comment.
Would it be better to take the bindInfo here and check if we are running on x86 platform, and if we are, then create an array of tagDBPARAMBINDINFO_x86 and Marshal the tagDBPARAMBINDINFO to the array of x86 structs?
Yes, that would be an option as well, especially given that this fix will need replicated in many more places. As you have said, it will result into extra data copies on x86, and the code will take very different path on x86 in general. We need to make sure to test it well.
maryamariyan
commented
Feb 14, 2020
closing in favor of PR #32207 |
Fixes: #981