Skip to content
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

Perf: Stop creating parameter prefixed names #1829

Merged
merged 1 commit into from
Apr 10, 2023
Merged

Perf: Stop creating parameter prefixed names #1829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
50 changes: 33 additions & 17 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4359,7 +4359,7 @@ private void ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDi
SqlParameter sqlParameter = rpc.userParams[index];
Debug.Assert(sqlParameter != null, "sqlParameter should not be null.");

if (sqlParameter.ParameterNameFixed.Equals(parameterName, StringComparison.Ordinal))
if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName,parameterName,StringComparison.Ordinal))
{
Debug.Assert(sqlParameter.CipherMetadata == null, "param.CipherMetadata should be null.");
sqlParameter.HasReceivedMetadata = true;
Expand Down Expand Up @@ -5453,7 +5453,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
{
if (rec.tdsType != TdsEnums.SQLBIGVARBINARY)
{
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.ParameterNameFixed, rec.tdsType, TdsEnums.SQLBIGVARBINARY);
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.GetPrefixedParameterName(), rec.tdsType, TdsEnums.SQLBIGVARBINARY);
}

// Decrypt the ciphertext
Expand Down Expand Up @@ -5483,7 +5483,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
}
catch (Exception e)
{
throw SQL.ParamDecryptionFailed(thisParam.ParameterNameFixed, null, e);
throw SQL.ParamDecryptionFailed(thisParam.GetPrefixedParameterName(), null, e);
}
}
else
Expand Down Expand Up @@ -5624,7 +5624,11 @@ private SqlParameter GetParameterForOutputValueExtraction(SqlParameterCollection
{
thisParam = parameters[i];
// searching for Output or InputOutput or ReturnValue with matching name
if (thisParam.Direction != ParameterDirection.Input && thisParam.Direction != ParameterDirection.ReturnValue && paramName == thisParam.ParameterNameFixed)
if (
thisParam.Direction != ParameterDirection.Input &&
thisParam.Direction != ParameterDirection.ReturnValue &&
SqlParameter.ParameterNamesEqual(paramName, thisParam.ParameterName,StringComparison.Ordinal)
)
{
foundParam = true;
break; // found it
Expand Down Expand Up @@ -5995,11 +5999,11 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

// Find the return value parameter (if any).
SqlParameter returnValueParameter = null;
foreach (SqlParameter parameter in parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep the full name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a variable with that name is created and used later in this scope. I don't want to explicitly re-use the same variable because it can leave opportunity for errors to creep in with casual refactorings in future. So I renamed the foreach version because it has the lower number of uses. Same with the other one you noted below.

foreach (SqlParameter param in parameters)
{
if (parameter.Direction == ParameterDirection.ReturnValue)
if (param.Direction == ParameterDirection.ReturnValue)
{
returnValueParameter = parameter;
returnValueParameter = param;
break;
}
}
Expand All @@ -6008,7 +6012,8 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// EXEC @returnValue = moduleName [parameters]
if (returnValueParameter != null)
{
execStatement.AppendFormat(@"{0}=", returnValueParameter.ParameterNameFixed);
SqlParameter.AppendPrefixedParameterName(execStatement, returnValueParameter.ParameterName);
execStatement.Append('=');
}

execStatement.Append(ParseAndQuoteIdentifier(storedProcedureName, false));
Expand All @@ -6019,6 +6024,7 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the first parameter
int index = 0;
int count = parameters.Count;
SqlParameter parameter;
if (count > 0)
{
// Skip the return value parameters.
Expand All @@ -6029,15 +6035,19 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

if (index < count)
{
parameter = parameters[index];
// Possibility of a SQL Injection issue through parameter names and how to construct valid identifier for parameters.
// Since the parameters comes from application itself, there should not be a security vulnerability.
// Also since the query is not executed, but only analyzed there is no possibility for elevation of privilege, but only for
// incorrect results which would only affect the user that attempts the injection.
execStatement.AppendFormat(@" {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(' ');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput)
if (parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput)
{
execStatement.AppendFormat(@" OUTPUT");
}
Expand All @@ -6050,14 +6060,18 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the rest of parameters
for (; index < count; index++)
{
if (parameters[index].Direction != ParameterDirection.ReturnValue)
parameter = parameters[index];
if (parameter.Direction != ParameterDirection.ReturnValue)
{
execStatement.AppendFormat(@", {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(", ");
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (
parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput
parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput
)
{
execStatement.AppendFormat(@" OUTPUT");
Expand Down Expand Up @@ -6091,9 +6105,11 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete

// add our separator for the ith parameter
if (fAddSeparator)
{
paramList.Append(',');
}

paramList.Append(sqlParam.ParameterNameFixed);
SqlParameter.AppendPrefixedParameterName(paramList, sqlParam.ParameterName);

MetaType mt = sqlParam.InternalMetaType;

Expand All @@ -6116,7 +6132,7 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete
string typeName = sqlParam.TypeName;
if (string.IsNullOrEmpty(typeName))
{
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.ParameterNameFixed);
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.GetPrefixedParameterName());
}
paramList.Append(ParseAndQuoteIdentifier(typeName, false /* is not UdtTypeName*/));

Expand Down
33 changes: 25 additions & 8 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9263,7 +9263,7 @@ private Task TDSExecuteRPCAddParameter(TdsParserStateObject stateObj, SqlParamet
}
}

WriteParameterName(param.ParameterNameFixed, stateObj, isAnonymous);
WriteParameterName(param.ParameterName, stateObj, isAnonymous);

// Write parameter status
stateObj.WriteByte(options);
Expand Down Expand Up @@ -9784,17 +9784,34 @@ private void ExecuteFlushTaskCallback(Task tsk, TdsParserStateObject stateObj, T
}
}


private void WriteParameterName(string parameterName, TdsParserStateObject stateObj, bool isAnonymous)
/// <summary>
/// Will check the parameter name for the required @ prefix and then write the correct prefixed
/// form and correct character length to the output buffer
/// </summary>
private void WriteParameterName(string rawParameterName, TdsParserStateObject stateObj, bool isAnonymous)
{
// paramLen
// paramName
if (!isAnonymous && !string.IsNullOrEmpty(parameterName))
if (!isAnonymous && !string.IsNullOrEmpty(rawParameterName))
{
Debug.Assert(parameterName.Length <= 0xff, "parameter name can only be 255 bytes, shouldn't get to TdsParser!");
int tempLen = parameterName.Length & 0xff;
stateObj.WriteByte((byte)tempLen);
WriteString(parameterName, tempLen, 0, stateObj);
int nameLength = rawParameterName.Length;
int totalLength = nameLength;
bool writePrefix = false;
if (nameLength > 0)
{
if (rawParameterName[0] != '@')
{
writePrefix = true;
totalLength += 1;
}
}
Debug.Assert(totalLength <= 0xff, "parameter name can only be 255 bytes, shouldn't get to TdsParser!");
stateObj.WriteByte((byte)(totalLength & 0xFF));
if (writePrefix)
{
WriteString("@", 1, 0, stateObj);
}
WriteString(rawParameterName, nameLength, 0, stateObj);
}
else
{
Expand Down
53 changes: 34 additions & 19 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4877,7 +4877,7 @@ private void ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDi
SqlParameter sqlParameter = rpc.userParams[index];
Debug.Assert(sqlParameter != null, "sqlParameter should not be null.");

if (sqlParameter.ParameterNameFixed.Equals(parameterName, StringComparison.Ordinal))
if (SqlParameter.ParameterNamesEqual(sqlParameter.ParameterName, parameterName, StringComparison.Ordinal))
{
Debug.Assert(sqlParameter.CipherMetadata == null, "param.CipherMetadata should be null.");
sqlParameter.HasReceivedMetadata = true;
Expand Down Expand Up @@ -6235,7 +6235,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
{
if (rec.tdsType != TdsEnums.SQLBIGVARBINARY)
{
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.ParameterNameFixed, rec.tdsType, TdsEnums.SQLBIGVARBINARY);
throw SQL.InvalidDataTypeForEncryptedParameter(thisParam.GetPrefixedParameterName(), rec.tdsType, TdsEnums.SQLBIGVARBINARY);
}

// Decrypt the ciphertext
Expand Down Expand Up @@ -6265,7 +6265,7 @@ internal void OnReturnValue(SqlReturnValue rec, TdsParserStateObject stateObj)
}
catch (Exception e)
{
throw SQL.ParamDecryptionFailed(thisParam.ParameterNameFixed, null, e);
throw SQL.ParamDecryptionFailed(thisParam.GetPrefixedParameterName(), null, e);
}
}
else
Expand Down Expand Up @@ -6458,7 +6458,11 @@ private SqlParameter GetParameterForOutputValueExtraction(SqlParameterCollection
{
thisParam = parameters[i];
// searching for Output or InputOutput or ReturnValue with matching name
if (thisParam.Direction != ParameterDirection.Input && thisParam.Direction != ParameterDirection.ReturnValue && paramName == thisParam.ParameterNameFixed)
if (
thisParam.Direction != ParameterDirection.Input &&
thisParam.Direction != ParameterDirection.ReturnValue &&
SqlParameter.ParameterNamesEqual(paramName, thisParam.ParameterName,StringComparison.Ordinal)
)
{
foundParam = true;
break; // found it
Expand Down Expand Up @@ -6846,11 +6850,11 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

// Find the return value parameter (if any).
SqlParameter returnValueParameter = null;
foreach (SqlParameter parameter in parameters)
foreach (SqlParameter param in parameters)
{
if (parameter.Direction == ParameterDirection.ReturnValue)
if (param.Direction == ParameterDirection.ReturnValue)
{
returnValueParameter = parameter;
returnValueParameter = param;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep the full name?

break;
}
}
Expand All @@ -6859,7 +6863,8 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// EXEC @returnValue = moduleName [parameters]
if (returnValueParameter != null)
{
execStatement.AppendFormat(@"{0}=", returnValueParameter.ParameterNameFixed);
SqlParameter.AppendPrefixedParameterName(execStatement, returnValueParameter.ParameterName);
execStatement.Append('=');
}

execStatement.Append(ParseAndQuoteIdentifier(storedProcedureName, false));
Expand All @@ -6870,6 +6875,7 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the first parameter
int index = 0;
int count = parameters.Count;
SqlParameter parameter;
if (count > 0)
{
// Skip the return value parameters.
Expand All @@ -6880,16 +6886,20 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto

if (index < count)
{
parameter = parameters[index];
// Possibility of a SQL Injection issue through parameter names and how to construct valid identifier for parameters.
// Since the parameters comes from application itself, there should not be a security vulnerability.
// Also since the query is not executed, but only analyzed there is no possibility for elevation of priviledge, but only for
// incorrect results which would only affect the user that attempts the injection.
execStatement.AppendFormat(@" {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(' ');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (
parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput
parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput
)
{
execStatement.AppendFormat(@" OUTPUT");
Expand All @@ -6903,14 +6913,18 @@ private SqlParameter BuildStoredProcedureStatementForColumnEncryption(string sto
// Append the rest of parameters
for (; index < count; index++)
{
if (parameters[index].Direction != ParameterDirection.ReturnValue)
parameter = parameters[index];
if (parameter.Direction != ParameterDirection.ReturnValue)
{
execStatement.AppendFormat(@", {0}={0}", parameters[index].ParameterNameFixed);
execStatement.Append(", ");
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);
execStatement.Append('=');
SqlParameter.AppendPrefixedParameterName(execStatement, parameter.ParameterName);

// InputOutput and Output parameters need to be marked as such.
if (
parameters[index].Direction == ParameterDirection.Output ||
parameters[index].Direction == ParameterDirection.InputOutput
parameter.Direction == ParameterDirection.Output ||
parameter.Direction == ParameterDirection.InputOutput
)
{
execStatement.AppendFormat(@" OUTPUT");
Expand Down Expand Up @@ -6942,9 +6956,10 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete

// add our separator for the ith parameter
if (fAddSeparator)
{
paramList.Append(',');

paramList.Append(sqlParam.ParameterNameFixed);
}
SqlParameter.AppendPrefixedParameterName(paramList, sqlParam.ParameterName);

MetaType mt = sqlParam.InternalMetaType;

Expand All @@ -6953,7 +6968,7 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete

// paragraph above doesn't seem to be correct. Server won't find the type
// if we don't provide a fully qualified name
paramList.Append(" ");
paramList.Append(' ');
if (mt.SqlDbType == SqlDbType.Udt)
{
string fullTypeName = sqlParam.UdtTypeName;
Expand All @@ -6967,7 +6982,7 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete
string typeName = sqlParam.TypeName;
if (ADP.IsEmpty(typeName))
{
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.ParameterNameFixed);
throw SQL.MustSetTypeNameForParam(mt.TypeName, sqlParam.GetPrefixedParameterName());
}
paramList.Append(ParseAndQuoteIdentifier(typeName, false /* is not UdtTypeName*/));

Expand Down
Loading