-
Notifications
You must be signed in to change notification settings - Fork 104
Fix invalid read in tsql_round_var function #3720
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
Fix invalid read in tsql_round_var function #3720
Conversation
When the rscale is negative and numeric value is 0 we can skip checking for numeric overflow as the base-NBASE digits will be empty for 0
Pull Request Test Coverage Report for Build 14620310275Details
💛 - Coveralls |
go | ||
~~START~~ | ||
numeric | ||
0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this also what SQL Server outputs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In sqlserver it includes decimal
1> select round(0.0, -1)
2> go
---
0.0
But currently in babelfish it doesn't show decimal with/without this change as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we figure out what the RCA is for this issue. I am just trying to confirm if this needs to be handled in tsql_round_var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are forcing it to 0 here
https://github.com/babelfish-for-postgresql/babelfish_extensions/blob/BABEL_5_X_DEV/contrib/babelfishpg_common/src/numeric.c#L672
So we have to handle it here I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have jira for correct scale from round function BABEL-5666.
Maybe this will be handled in resolve_numeric_typmod_from_exp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sharathbp could we add this query in the comments of that jira so we don't miss while fixing that change.
go | ||
~~START~~ | ||
numeric | ||
0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we figure out what the RCA is for this issue. I am just trying to confirm if this needs to be handled in tsql_round_var
eb1213e
into
babelfish-for-postgresql:BABEL_5_X_DEV
Description
To detect for overflow and underflow we need to determine total integral digits which in-turn uses leading digits in the digits array. But digits array will be empty in case of 0 numeric values which leads to invalid read.
When the rscale is negative and numeric value is 0 we can skip checking for numeric overflow as the base-NBASE digits will be empty for 0.
Issues Resolved
BABEL-5546
Test Scenarios Covered
Use case based -
Yes
Boundary conditions -
Yes
Arbitrary inputs -
Yes
Negative test cases -
Yes
Minor version upgrade tests -
NA
Major version upgrade tests -
NA
Performance tests -
NA
Tooling impact -
NA
Client tests -
NA
Check List
By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.