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

Fix possible race condition #702

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

afrizaloky
Copy link

@afrizaloky afrizaloky commented Apr 29, 2024

Fix possible race conditions.

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

Checklist

@afrizaloky afrizaloky changed the title Fix possible race conditions Fix possible race condition Apr 29, 2024
@Lagovas
Copy link
Collaborator

Lagovas commented Apr 29, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

Lagovas commented Apr 30, 2024

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
Copy link
Author

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

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.

None yet

2 participants