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.

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

Draft | Addressing the issue with wrong exception when column decryption fails. #1897

Closed
wants to merge 3 commits into from

Conversation

JRahnama
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Base: 70.70% // Head: 69.91% // Decreases project coverage by -0.80% ⚠️

Coverage data is based on head (180d081) compared to base (0156df2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1897      +/-   ##
==========================================
- Coverage   70.70%   69.91%   -0.80%     
==========================================
  Files         292      292              
  Lines       61732    61733       +1     
==========================================
- Hits        43647    43159     -488     
- Misses      18085    18574     +489     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.18% <100.00%> (+0.06%) ⬆️
netfx 67.86% <ø> (-1.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs 96.59% <ø> (ø)
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 73.39% <100.00%> (-0.04%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 27.57% <0.00%> (-39.24%) ⬇️
...c/Microsoft/Data/Interop/SNINativeMethodWrapper.cs 66.91% <0.00%> (-9.53%) ⬇️
...Microsoft/Data/ProviderBase/DbConnectionFactory.cs 17.04% <0.00%> (-3.41%) ⬇️
...icrosoft/Data/ProviderBase/DbConnectionInternal.cs 78.22% <0.00%> (-2.42%) ⬇️
...e/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs 62.62% <0.00%> (-2.32%) ⬇️
...soft/Data/SqlClient/TdsParserStateObjectManaged.cs 84.02% <0.00%> (-1.39%) ⬇️
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs 68.85% <0.00%> (-0.82%) ⬇️
...osoft/Data/SqlClient/TdsParserStateObjectNative.cs 86.84% <0.00%> (-0.53%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

{
if (token == TdsEnums.SQLERROR)
if (token == TdsEnums.SQLERROR || token == TdsEnums.SQLDEBUG_CMD) // This the value of 0x60 send fromm TDS Control token
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what the comment is trying to communicate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the comment since this was a draft PR and I needed to make a change to run the pipelines I did not pay much attention to it.

The specs do not provide any info on that TDS token stream. At first, I did not notice that 0x60 is available in TDSEnums. basically, the issue was raised by PS team. They tried to provide an invalid token(AE token not TDS header token) to decrypt an encrypted column. Server returns 96 instead of error token (0xaa).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're getting it from the server then either they need to update the spec or stop sending it. I think updating the spec is easier.

@@ -149,7 +149,7 @@ internal static class TdsEnums
public const byte SQLDONEINPROC = 0xff;
public const byte SQLOFFSET = 0x78;
public const byte SQLORDER = 0xa9;
public const byte SQLDEBUG_CMD = 0x60;
public const byte SQLDEBUG_CMD = 0x60; // This needs more info
Copy link
Contributor

Choose a reason for hiding this comment

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

Who needs to provide the info? the SQL Server team? The spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to provide more info and add it here 😄 . This is just a replacement for more info and triggering pipelines.

@lcheunglci
Copy link
Contributor

Should we close this PR since the fix in #1948 address this issue as well?

@JRahnama
Copy link
Contributor Author

Should we close this PR since the fix in #1948 address this issue as well?

I forgot. I should close this.

@JRahnama JRahnama closed this Mar 23, 2023
@JRahnama JRahnama deleted the PSIssue branch September 8, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants