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

sql: fix pgwire parsing certain binary encodings of numeric values an order of magnitude off #35115

Merged
merged 1 commit into from May 29, 2019

Conversation

Projects
None yet
4 participants
@sorccu
Copy link
Contributor

commented Feb 21, 2019

Some numeric values can be encoded in multiple ways on the wire. For example, 1000000 may be encoded as:

{
  Ndigits: 1,
  Weight: 1,
  Sign: 0,
  Dscale: 0,
  digits: []byte{0, 100}
}

This specific case works and is tested by TestEncodings. However, an alternate encoding for the same value exists:

{
  Ndigits: 2,
  Weight: 1,
  Sign: 0,
  Dscale: 0,
  digits: []byte{0, 100, 0, 0}
}

This encoding results in Cockroach producing a value that's an order of magnitude larger than intended (10000000). Importantly, this is the encoding used by Diesel in the Rust world.

The issue was found in DecodeOidDatum, where nextDigit() takes care of appending the right number of zeroes to the value, up to 4 total per i16 group. Since nextDigit() can complete a zero value entirely by itself, strconv.AppendUint is correctly skipped if the value of the group is 0, as that would add a fifth digit. However, the last group of digits is handled separately from the rest. That code was missing special handling for the 0 value, resulting in 5 zero digits being produced for the final group. The issue was remedied by introducing the same check done for the other digit groups.

Test cases for failing examples were added in the form of TestExoticNumericEncodings. Unfortunately there did not seem to be a way to merge these with TestEncodings as its test cases are generated programmatically, thus limiting the possible values to the ones chosen by Postgres, which were already fine.

sql: fix pgwire parsing certain binary encodings of numeric values an
order of magnitude off

Some numeric values can be encoded in multiple ways on the wire. For
example, 1000000 may be encoded as:

    {
      Ndigits: 1,
      Weight: 1,
      Sign: 0,
      Dscale: 0,
      digits: []byte{0, 100}
    }

This specific case works and is tested by `TestEncodings`. `However`, an
alternate encoding for the same value exists:

    {
      Ndigits: 2,
      Weight: 1,
      Sign: 0,
      Dscale: 0,
      digits: []byte{0, 100, 0, 0}
    }

This encoding results in Cockroach producing a value that's an order of
magnitude larger than intended (10000000). Importantly, this is the
encoding used by Diesel in the Rust world.

The issue was found in `DecodeOidDatum`, where `nextDigit()` takes care
of appending the right number of zeroes to the value, up to 4 total per
`i16` group. Since `nextDigit()` can complete a zero value entirely by
itself, `strconv.AppendUint` is correctly skipped if the value of the
group is 0, as that would add a fifth digit. However, the last group of
digits is handled separately from the rest. That code was missing
special handling for the 0 value, resulting in 5 zero digits being
produced for the final group. The issue was remedied by introducing the
same check done for the other digit groups.

Test cases for failing examples were added in the form of
`TestExoticNumericEncodings`. Unfortunately there did not seem to be a
way to merge these with `TestEncodings` as its test cases are generated
programmatically, thus limiting the possible values to the ones chosen
by Postgres, which were already fine.

Release note (bug fix): Certain binary encodings of numeric/decimal
values no longer result in values that are an order of magnitude off.

@sorccu sorccu requested a review from cockroachdb/sql-wiring-prs as a code owner Feb 21, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

This change is Reviewable

@justinj justinj requested a review from mjibson May 29, 2019

@justinj

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@sorccu sorry this PR appears to have slipped through the cracks! Your contribution is much appreciated. I've tagged in our pgwire expert to take a look.

@mjibson

This comment has been minimized.

Copy link
Member

commented May 29, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 29, 2019

Merge #35115
35115: sql: fix pgwire parsing certain binary encodings of numeric values an order of magnitude off r=mjibson a=sorccu

Some numeric values can be encoded in multiple ways on the wire. For example, 1000000 may be encoded as:

    {
      Ndigits: 1,
      Weight: 1,
      Sign: 0,
      Dscale: 0,
      digits: []byte{0, 100}
    }

This specific case works and is tested by `TestEncodings`. However, an alternate encoding for the same value exists:

    {
      Ndigits: 2,
      Weight: 1,
      Sign: 0,
      Dscale: 0,
      digits: []byte{0, 100, 0, 0}
    }

This encoding results in Cockroach producing a value that's an order of magnitude larger than intended (10000000). Importantly, this is the encoding used by Diesel in the Rust world.

The issue was found in `DecodeOidDatum`, where `nextDigit()` takes care of appending the right number of zeroes to the value, up to 4 total per `i16` group. Since `nextDigit()` can complete a zero value entirely by itself, `strconv.AppendUint` is correctly skipped if the value of the group is 0, as that would add a fifth digit. However, the last group of digits is handled separately from the rest. That code was missing special handling for the 0 value, resulting in 5 zero digits being produced for the final group. The issue was remedied by introducing the same check done for the other digit groups.

Test cases for failing examples were added in the form of `TestExoticNumericEncodings`. Unfortunately there did not seem to be a way to merge these with `TestEncodings` as its test cases are generated programmatically, thus limiting the possible values to the ones chosen by Postgres, which were already fine.

Co-authored-by: Simo Kinnunen <simo@telco.in>
@craig

This comment has been minimized.

Copy link

commented May 29, 2019

Build succeeded

@craig craig bot merged commit db7f834 into cockroachdb:master May 29, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.