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

cherry-pick 1.1: pgwire: fix issue with re-using old buffer data #22262

Merged
merged 1 commit into from Jan 31, 2018

Conversation

Projects
None yet
4 participants
@justinj
Member

justinj commented Jan 31, 2018

Cherry-pick of #20461.

cc @cockroachdb/release

Release notes: fix issue with stale buffer data when using the binary
format for arrays.

Fixes #20372.
Fixes #19669.

This commit fixes an issue involving passing a bytes.Buffer by value
which would cause old buffered data for arrays to be re-used.

The bug here was somewhat subtle and had to do with copying a
bytes.Buffer by value whose slice header pointed to its fixed-size array
used for small allocations, and then re-assigning the original buffer,
causing the fixed-size array to be overwritten and the buffered value
changed. A reduced version of the issue can be seen here:

https://play.golang.org/p/4-v_AeqYtR

cherry-pick 1.1: pgwire: fix issue with re-using old buffer data
Release notes: fix issue with stale buffer data when using the binary
format for arrays.

Fixes #20372.
Fixes #19669.

This commit fixes an issue involving passing a bytes.Buffer by value
which would cause old buffered data for arrays to be re-used.

The bug here was somewhat subtle and had to do with copying a
bytes.Buffer by value whose slice header pointed to its fixed-size array
used for small allocations, and then *re-assigning* the original buffer,
causing the fixed-size array to be overwritten and the buffered value
changed. A reduced version of the issue can be seen here:

https://play.golang.org/p/4-v_AeqYtR

@justinj justinj requested a review from knz Jan 31, 2018

@justinj justinj requested a review from cockroachdb/sql-wiring-prs as a code owner Jan 31, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Jan 31, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Jan 31, 2018

This change is Reviewable

@knz

knz approved these changes Jan 31, 2018

thank you

@justinj justinj merged commit 6072118 into cockroachdb:release-1.1 Jan 31, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
code-review/reviewable Review complete: 0 of 0 LGTMs obtained
Details
license/cla Contributor License Agreement is signed.
Details

@justinj justinj deleted the justinj:cp-array-pgwire branch Jan 31, 2018

@tlvenn

This comment has been minimized.

Show comment
Hide comment
@tlvenn

tlvenn Jan 31, 2018

Contributor

thank you @justinj !

Contributor

tlvenn commented Jan 31, 2018

thank you @justinj !

@tlvenn

This comment has been minimized.

Show comment
Hide comment
@tlvenn

tlvenn Mar 15, 2018

Contributor

Hi @justinj , just wanted to double confirm that this fix was included in the 1.1.6 release ?

I have upgraded to it on my machine:

Build Tag:    v1.1.6
Build Time:   2018/03/12 22:33:39
Distribution: CCL
Platform:     darwin amd64
Go Version:   go1.10
C Compiler:   4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)
Build SHA-1:  f6b7567d85d54689a7d1b61907336d1bc72b09a4
Build Type:   development

Yet I have an error as before from time to time:

[error] GenServer #PID<0.594.0> terminating
** (DBConnection.ConnectionError) client #PID<0.640.0> stopped: ** (MatchError) no match of right hand side value: <<48, 49, 49, 52, 121, 95, 105, 110, 116, 118, 118, 110, 116, 118, 116, 118, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>
    (postgrex) lib/postgrex/type_module.ex:717: Postgrex.DefaultTypes."Elixir.Postgrex.Extensions.Array"/8
    (postgrex) lib/postgrex/protocol.ex:1982: Postgrex.Protocol.rows_recv/4
    (postgrex) lib/postgrex/protocol.ex:1329: Postgrex.Protocol.execute_recv/5
    (db_connection) lib/db_connection.ex:958: DBConnection.handle/4
Contributor

tlvenn commented Mar 15, 2018

Hi @justinj , just wanted to double confirm that this fix was included in the 1.1.6 release ?

I have upgraded to it on my machine:

Build Tag:    v1.1.6
Build Time:   2018/03/12 22:33:39
Distribution: CCL
Platform:     darwin amd64
Go Version:   go1.10
C Compiler:   4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)
Build SHA-1:  f6b7567d85d54689a7d1b61907336d1bc72b09a4
Build Type:   development

Yet I have an error as before from time to time:

[error] GenServer #PID<0.594.0> terminating
** (DBConnection.ConnectionError) client #PID<0.640.0> stopped: ** (MatchError) no match of right hand side value: <<48, 49, 49, 52, 121, 95, 105, 110, 116, 118, 118, 110, 116, 118, 116, 118, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>
    (postgrex) lib/postgrex/type_module.ex:717: Postgrex.DefaultTypes."Elixir.Postgrex.Extensions.Array"/8
    (postgrex) lib/postgrex/protocol.ex:1982: Postgrex.Protocol.rows_recv/4
    (postgrex) lib/postgrex/protocol.ex:1329: Postgrex.Protocol.execute_recv/5
    (db_connection) lib/db_connection.ex:958: DBConnection.handle/4
@justinj

This comment has been minimized.

Show comment
Hide comment
@justinj

justinj Mar 15, 2018

Member

It certainly should be in 1.1.6 - is it happening with the same frequency as before? Are you able to test on our most recent 2.0 beta and see if you see similar things there?

Member

justinj commented Mar 15, 2018

It certainly should be in 1.1.6 - is it happening with the same frequency as before? Are you able to test on our most recent 2.0 beta and see if you see similar things there?

@tlvenn

This comment has been minimized.

Show comment
Hide comment
@tlvenn

tlvenn Mar 15, 2018

Contributor

No it definitely seems a lot rarer now and i could not reproduce it at all (as far as I could see) with the recent 2.0 beta builds.

Contributor

tlvenn commented Mar 15, 2018

No it definitely seems a lot rarer now and i could not reproduce it at all (as far as I could see) with the recent 2.0 beta builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment