Skip to content

OLEDB: Fix for AV Exception on x86 architecture #32150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
maryamariyan wants to merge 3 commits into from
Closed

OLEDB: Fix for AV Exception on x86 architecture #32150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be68b1b
Fixing AccessViolationException on x86 architecture
maryamariyan Feb 11, 2020
e3886b6
Update src/libraries/System.Data.OleDb/src/DbBindings.cs
maryamariyan Feb 12, 2020
8dcf98f
implicit check for x86
maryamariyan Feb 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 66 additions & 15 deletions src/libraries/System.Data.OleDb/src/DbBindings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@

using System.Data.Common;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Text;

namespace System.Data.OleDb
{
internal sealed class Bindings
{
private static readonly bool s_runningOnX86 = RuntimeInformation.ProcessArchitecture == Architecture.X86;
Copy link
Copy Markdown
Member

@danmoseley danmoseley Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@jkotas jkotas Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private readonly tagDBPARAMBINDINFO[] _bindInfo;
private readonly tagDBPARAMBINDINFO_x86[] _bindInfo_x86;
private readonly tagDBBINDING[] _dbbindings;
private readonly tagDBCOLUMNACCESS[] _dbcolumns;

Expand Down Expand Up @@ -42,7 +45,10 @@ private Bindings(int count)

internal Bindings(OleDbParameter[] parameters, int collectionChangeID) : this(parameters.Length)
{
_bindInfo = new tagDBPARAMBINDINFO[parameters.Length];
if (s_runningOnX86)
_bindInfo_x86 = new tagDBPARAMBINDINFO_x86[parameters.Length];
else
_bindInfo = new tagDBPARAMBINDINFO[parameters.Length];
_parameters = parameters;
_collectionChangeID = collectionChangeID;
_ifIRowsetElseIRow = true;
Expand All @@ -54,6 +60,11 @@ internal Bindings(OleDbDataReader dataReader, bool ifIRowsetElseIRow, int count)
_ifIRowsetElseIRow = ifIRowsetElseIRow;
}

internal tagDBPARAMBINDINFO_x86[] BindInfo_x86
{
get { return _bindInfo_x86; }
}

internal tagDBPARAMBINDINFO[] BindInfo
{
get { return _bindInfo; }
Expand Down Expand Up @@ -100,41 +111,61 @@ internal bool ForceRebind
// tagDBPARAMBINDINFO member access
internal IntPtr DataSourceType
{
//get { return _bindInfo[_index].pwszDataSourceType; }
set
{
_bindInfo[_index].pwszDataSourceType = value;
if (s_runningOnX86)
_bindInfo_x86[_index].pwszDataSourceType = value;
else
_bindInfo[_index].pwszDataSourceType = value;
}
}
internal IntPtr Name
{
//get { return _bindInfo[_index].pwszName; }
set
{
_bindInfo[_index].pwszName = value;
if (s_runningOnX86)
Copy link
Copy Markdown
Member

@danmoseley danmoseley Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ternary expression might be neater for these (?)

_bindInfo_x86[_index].pwszName = value;
else
_bindInfo[_index].pwszName = value;
}
}
internal IntPtr ParamSize
{
get
{
if (null != _bindInfo)
if (s_runningOnX86)
{
if (null != _bindInfo_x86)
{
return _bindInfo_x86[_index].ulParamSize;
}
return IntPtr.Zero;
Comment thread
maryamariyan marked this conversation as resolved.
}
else
{
return _bindInfo[_index].ulParamSize;
if (null != _bindInfo)
{
return _bindInfo[_index].ulParamSize;
}
return IntPtr.Zero;
}
return IntPtr.Zero;
}
set
{
_bindInfo[_index].ulParamSize = value;
if (s_runningOnX86)
_bindInfo_x86[_index].ulParamSize = value;
else
_bindInfo[_index].ulParamSize = value;
}
}
internal int Flags
{
//get { return _bindInfo[_index].dwFlag; }
set
{
_bindInfo[_index].dwFlags = value;
if (s_runningOnX86)
_bindInfo_x86[_index].dwFlags = value;
else
_bindInfo[_index].dwFlags = value;
}
}

Expand Down Expand Up @@ -223,9 +254,19 @@ internal byte Precision
#endif
set
{
if (null != _bindInfo)
if (s_runningOnX86)
{
_bindInfo[_index].bPrecision = value;
if (null != _bindInfo_x86)
{
_bindInfo_x86[_index].bPrecision = value;
}
}
else
{
if (null != _bindInfo)
{
_bindInfo[_index].bPrecision = value;
}
}
_dbbindings[_index].bPrecision = value;
_dbcolumns[_index].bPrecision = value;
Expand All @@ -238,9 +279,19 @@ internal byte Scale
#endif
set
{
if (null != _bindInfo)
if (s_runningOnX86)
{
_bindInfo[_index].bScale = value;
if (null != _bindInfo_x86)
{
_bindInfo_x86[_index].bScale = value;
}
}
else
{
if (null != _bindInfo)
{
_bindInfo[_index].bScale = value;
}
}
_dbbindings[_index].bScale = value;
_dbcolumns[_index].bScale = value;
Expand Down
36 changes: 33 additions & 3 deletions src/libraries/System.Data.OleDb/src/OleDbCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,14 @@ private void CreateAccessor()

bindings.AllocateForAccessor(null, 0, 0);

ApplyParameterBindings(commandWithParameters, bindings.BindInfo);
if (bindings.BindInfo_x86 != null)
{
ApplyParameterBindings(commandWithParameters, bindings.BindInfo_x86);
Copy link
Copy Markdown
Member

@jkotas jkotas Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else
{
ApplyParameterBindings(commandWithParameters, bindings.BindInfo);
}

UnsafeNativeMethods.IAccessor iaccessor = IAccessor();
OleDbHResult hr = bindings.CreateAccessor(iaccessor, ODB.DBACCESSOR_PARAMETERDATA);
Expand All @@ -411,14 +418,37 @@ private void CreateAccessor()
_dbBindings = bindings;
}

private void ApplyParameterBindings(UnsafeNativeMethods.ICommandWithParameters commandWithParameters, tagDBPARAMBINDINFO[] bindInfo)
private unsafe void ApplyParameterBindings(UnsafeNativeMethods.ICommandWithParameters commandWithParameters, tagDBPARAMBINDINFO[] bindInfo)
{
IntPtr[] ordinals = new IntPtr[bindInfo.Length];
for (int i = 0; i < ordinals.Length; ++i)
{
ordinals[i] = (IntPtr)(i + 1);
}
OleDbHResult hr = commandWithParameters.SetParameterInfo((IntPtr)bindInfo.Length, ordinals, bindInfo);
OleDbHResult hr;
fixed (tagDBPARAMBINDINFO* p = &bindInfo[0])
Copy link
Copy Markdown
Contributor

@saurabh500 saurabh500 Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

private unsafe void ApplyParameterBindings(UnsafeNativeMethods.ICommandWithParameters commandWithParameters, tagDBPARAMBINDINFO[] bindInfo)
        {
            IntPtr[] ordinals = new IntPtr[bindInfo.Length];
            for (int i = 0; i < ordinals.Length; ++i)
            {
                ordinals[i] = (IntPtr)(i + 1);
            }

            OleDbHResult hr;

            if (s_runningOnX86)
            {
                tagDBPARAMBINDINFO_x86[] bindInfo_x86 = new tagDBPARAMBINDINFO_x86[bindInfo.Length];
                for (int i = 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.OleDbHResult SetParameterInfo(
                [In] IntPtr cParams,
                [In, MarshalAs(UnmanagedType.LPArray)] IntPtr[] rgParamOrdinals,
                [In] IntPtr rgParamBindInfo);

as @maryamariyan has already done?

There would be data duplication and manipulation for x86 scenario, but it does simplify the code greatly.

Copy link
Copy Markdown
Member

@jkotas jkotas Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
hr = commandWithParameters.SetParameterInfo((IntPtr)bindInfo.Length, ordinals, (IntPtr)p);
}

if (hr < 0)
{
ProcessResults(hr);
}
}

private unsafe void ApplyParameterBindings(UnsafeNativeMethods.ICommandWithParameters commandWithParameters, tagDBPARAMBINDINFO_x86[] bindInfo_x86)
{
IntPtr[] ordinals = new IntPtr[bindInfo_x86.Length];
for (int i = 0; i < ordinals.Length; ++i)
{
ordinals[i] = (IntPtr)(i + 1);
}
OleDbHResult hr;
fixed (tagDBPARAMBINDINFO_x86* p = &bindInfo_x86[0])
{
hr = commandWithParameters.SetParameterInfo((IntPtr)bindInfo_x86.Length, ordinals, (IntPtr)p);
}

if (hr < 0)
{
Expand Down
30 changes: 27 additions & 3 deletions src/libraries/System.Data.OleDb/src/OleDbStruct.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,35 @@ typedef struct tagDBPARAMBINDINFO {
}
#endif

#if (WIN32 && !ARCH_arm)
Copy link
Copy Markdown
Member

@jkotas jkotas Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the same ifdef on many other structs in this file. Do all of them need to be fixed?

Copy link
Copy Markdown
Member

@danmoseley danmoseley Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that or we shoukd remove the attributes/defines for them

Copy link
Copy Markdown
Contributor

@saurabh500 saurabh500 Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@jkotas jkotas Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

[StructLayoutAttribute(LayoutKind.Sequential, Pack = 2)]
Copy link
Copy Markdown
Member

@jkotas jkotas Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment highlighting the difference in the x86 version may be useful

#else
[StructLayout(LayoutKind.Sequential, Pack = 8)]
internal struct tagDBPARAMBINDINFO_x86
{
internal IntPtr pwszDataSourceType;
internal IntPtr pwszName;
internal IntPtr ulParamSize;
internal int dwFlags;
internal byte bPrecision;
internal byte bScale;

#if DEBUG
Copy link
Copy Markdown
Member

@danmoseley danmoseley Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@maryamariyan maryamariyan Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to keep the style in the rest of this file. Otherwise I don't mind just removing the whole thing

public override string ToString()
{
StringBuilder builder = new StringBuilder();
builder.Append("tagDBPARAMBINDINFO_x86").Append(Environment.NewLine);
if (IntPtr.Zero != pwszDataSourceType)
{
builder.Append("pwszDataSourceType =").Append(Marshal.PtrToStringUni(pwszDataSourceType)).Append(Environment.NewLine);
}
builder.Append("\tulParamSize =" + ulParamSize.ToInt64().ToString(CultureInfo.InvariantCulture)).Append(Environment.NewLine);
builder.Append("\tdwFlags =0x" + dwFlags.ToString("X4", CultureInfo.InvariantCulture)).Append(Environment.NewLine);
builder.Append("\tPrecision =" + bPrecision.ToString(CultureInfo.InvariantCulture)).Append(Environment.NewLine);
builder.Append("\tScale =" + bScale.ToString(CultureInfo.InvariantCulture)).Append(Environment.NewLine);
return builder.ToString();
}
#endif
}

[StructLayout(LayoutKind.Sequential, Pack = 8)]
internal struct tagDBPARAMBINDINFO
{
internal IntPtr pwszDataSourceType;
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ HRESULT SetParameterInfo(
System.Data.OleDb.OleDbHResult SetParameterInfo(
[In] IntPtr cParams,
[In, MarshalAs(UnmanagedType.LPArray)] IntPtr[] rgParamOrdinals,
[In, MarshalAs(UnmanagedType.LPArray, ArraySubType = UnmanagedType.Struct)] System.Data.OleDb.tagDBPARAMBINDINFO[] rgParamBindInfo);
[In] IntPtr rgParamBindInfo);
}

[Guid("2206CCB1-19C1-11D1-89E0-00C04FD7A829"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown), ComImport, SuppressUnmanagedCodeSecurity]
Expand Down