Fix possible race condition

[afrizaloky]

Fix possible race conditions.

Since container/list is not thread safe, so we need to lock the list during Add and Remove operations.

Checklist

• [ ] Change is covered by automated tests
• [ ] The coding guidelines are followed
• [ ] Public API has proper documentation in the Acra documentation site or has PR on documentation repository with new changes
• [ ] CHANGELOG.md is updated (in case of notable or breaking changes)
• [ ] CHANGELOG_DEV.md is updated
• [ ] Benchmark results are attached (if applicable)
• are up-to-date (in case of API changes)

[Lagovas]

Did you get any deadlock or race conditions in practice? This structure is used in the context of one connection and processed only by two goroutines working sequentially. Due to sequential processing and no shared data between several connections, we don't use extra locks. We have next flow: [new connection] -> [new protocol & connection related structs] -> [2 new gorotuines: client & db connection processing] -> [end of processing in 2 goroutines]

Due to database protocols being synchronous and sequential (except extra cases of notifications that out of scope of processing by Acra), at first we send request, and then we get the response. So, two goroutines that handle client & db connections work sequentially and wouldn't do anything simultaneously due to protocols that don't support it.

So, we don't need here any mutes that will only slow down processing

[afrizaloky]

I don't know that the "list" is thread safe by design, but in my test, i got this panic

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xc6da19]

goroutine 22 [running]:
container/list.(*List).insert(...)
        /go/src/container/list/list.go:96
container/list.(*List).insertValue(...)
        /go/src/container/list/list.go:104
container/list.(*List).PushBack(...)
        /go/src/container/list/list.go:152
github.com/cossacklabs/acra/decryptor/postgresql.(*pendingPacketsList).Add(0xc0000142b0, {0x10fd560?, 0xc0007c45d0?})
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pending_packets.go:53 +0x359
github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).handleClientPacket(0xc00020a000, {0x1337408?, 0xc000560e40?}, 0x117d9aa?, 0x6?)
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:360 +0x605
github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).ProxyClientConnection(0xc00020a000, {0x1337408?, 0xc0001fedb0?}, 0xc00004c120?)
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:299 +0xab9
created by github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).ProxyDatabaseConnection
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:780 +0xe7e
exit status 2

[afrizaloky]

Let's say your design is like that, maybe there is another issue in the implementation. Because i don't know the design, i just put mutex there

[Lagovas]

I don't know that the "list" is thread safe by design, but in my test, i got this panic

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xc6da19]

goroutine 22 [running]:
container/list.(*List).insert(...)
        /go/src/container/list/list.go:96
container/list.(*List).insertValue(...)
        /go/src/container/list/list.go:104
container/list.(*List).PushBack(...)
        /go/src/container/list/list.go:152
github.com/cossacklabs/acra/decryptor/postgresql.(*pendingPacketsList).Add(0xc0000142b0, {0x10fd560?, 0xc0007c45d0?})
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pending_packets.go:53 +0x359
github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).handleClientPacket(0xc00020a000, {0x1337408?, 0xc000560e40?}, 0x117d9aa?, 0x6?)
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:360 +0x605
github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).ProxyClientConnection(0xc00020a000, {0x1337408?, 0xc0001fedb0?}, 0xc00004c120?)
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:299 +0xab9
created by github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).ProxyDatabaseConnection
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:780 +0xe7e
exit status 2

thanks for the traceback. can you tell which tool or language + library you used for PostgreSQL?

[afrizaloky]

I don't know that the "list" is thread safe by design, but in my test, i got this panic

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xc6da19]

goroutine 22 [running]:
container/list.(*List).insert(...)
        /go/src/container/list/list.go:96
container/list.(*List).insertValue(...)
        /go/src/container/list/list.go:104
container/list.(*List).PushBack(...)
        /go/src/container/list/list.go:152
github.com/cossacklabs/acra/decryptor/postgresql.(*pendingPacketsList).Add(0xc0000142b0, {0x10fd560?, 0xc0007c45d0?})
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pending_packets.go:53 +0x359
github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).handleClientPacket(0xc00020a000, {0x1337408?, 0xc000560e40?}, 0x117d9aa?, 0x6?)
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:360 +0x605
github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).ProxyClientConnection(0xc00020a000, {0x1337408?, 0xc0001fedb0?}, 0xc00004c120?)
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:299 +0xab9
created by github.com/cossacklabs/acra/decryptor/postgresql.(*PgProxy).ProxyDatabaseConnection
        /go/src/github.com/cossacklabs/acra/decryptor/postgresql/pg_decryptor.go:780 +0xe7e
exit status 2

thanks for the traceback. can you tell which tool or language + library you used for PostgreSQL?

I use this library npgsql

[Zhaars]

Thank you very much for reaching out @afrizaloky . Could you please provide an example of how to reproduce the panic? Ideally, this could be a Docker Compose file with configured Acra and an example script with SQL query using the library you are using that leads to the panic.

[afrizaloky]

But first, i need you to assess this test, because unlikely this test will be implemented in production code. This code run in single thread. This is the pseudo code:

Infinite loop: open connection select small data from table close connection End loop if crash

If you want to fix this problem, i will containerize the test. But if you don't want, i will close this issue.

NOTE: after this PR, with the same test, i found race condition on ClientSession.data