Fix for #1104 [free PgCredentials] #1105
Conversation
kill_database must call slab_free(credentials_cache, db->forced_user_credentials) but not slab_free(user_cache, ...). Because db->forced_user_credentials is allocated with slab_alloc(credentials_cache). See force_user_credentials function.
…ser_cache credentials_node_release is used only with PgDatabase::user_tree. PgDatabase::user_tree is filled in add_dynamic_credentials. add_dynamic_credentials allocates new credentials through slab_alloc(credentials_cache). So, credentials_node_release must use slab_free(credentials_cache, user).
dmitry-lipetsk
changed the title
|
Oooff, that's not good... Thanks for this fix. Any chance you can add a test that triggers the reported crash? @emelsimsek @eulerto I think this is probably worth a patch release after this is merged. Let's not merge any other (non-critical) PRs so we can create that patch release directly from the master brach. |
|
Ah I see now that you added the test in #1114, could you move that test here? |
If "test_qa_gh1104" is not OK, please change it with any another correct name.
|
Cirrus CI / Linux (Debian/Ubuntu) CFLAGS:-O0 -g ENABLE_VALGRIND:yes PGVERSION:17
--- WIN-FIX1104 https://github.com/pgbouncer/pgbouncer/pull/1105/checks?check_run_id=27239176206
It is bad... pgbouncer has fix for #1104. --- WIN-FIX1103 https://github.com/pgbouncer/pgbouncer/pull/1106/checks?check_run_id=27239898084
It is another test but it has the same problem. pgbouncer has fix for #1103. --- WIN-QA1104 Cirrus CI / Windows - QA test for issue #1104 https://github.com/pgbouncer/pgbouncer/pull/1114/checks?check_run_id=27215709319
QA test uses unmodified pgbouncer. Source code has the one point with string "not allowed": Line 1718 in e0937fd |
I think it is a reason for "not allowed" in test on Windows. Will try.
Or it solves the problem with Windows or I will start testing this on my local Windows :)
|
Hello, I have a little problem with configuration of development environment on Windows to research and fix a problem in test_ga_gh1104. So, any help with it will be appreciative. |
auth_type = trust admin_users = pgbouncer It was tested on Windows and Ubuntu. Amen.
It is enough to crash.
|
Done. |
After the refactoring of the user managament in pgbouncer#1052, we incorrectly freed credentials by calling `slab_free` on a user_cache, instead of `credentials_cache` in two places. This could later cause segfaults because ththe user type is larger than the credentials type, so out of bound reads would be possible when the credential that was put into the user_cache was taken out again and used as a user instead. (cherry picked from commit 6c8a1df)
After the refactoring of the user managament in #1052, we incorrectly freed credentials by calling `slab_free` on a user_cache, instead of `credentials_cache` in two places. This could later cause segfaults because ththe user type is larger than the credentials type, so out of bound reads would be possible when the credential that was put into the user_cache was taken out again and used as a user instead. (cherry picked from commit 6c8a1df)
2 participants
After the refactoring of the user managament in #1052, we incorrectly freed credentials by calling
slab_freeon a user_cache, instead ofcredentials_cachein two places. This could later cause segfaults because ththe user type is larger than the credentials type, so out of bound reads would be possible when the credential that was put into the user_cache was taken out again and used as a user instead.