Skip to content

Conversation

@stoops
Copy link

@stoops stoops commented Nov 4, 2025

Observed that the tls_session_init() function only initializes the KS_PRIMARY key and not the KS_LAME_DUCK key and the tls_session_free() function will free ALL keys always. This is a mis-match in the init-free callback chain which can potentially cause problems. For example, if a client connects and then disconnects before the renegotiation window, the lame duck key may not be initialized and then the key_state_free() function will be called which will then call the free_buf() function last (this may likely only operate on a NULL pointer and may indeed also check for this but it is not considered a good quality code path and practice in general to have)! Thanks.

Captured some example gdb log output using this example PR: #907

Before:

2025-11-17 12:12:50 DEBUG KEY FREE PTR [0xaaaaaac44270][0xaaaaaac3ffe0][0xaaaaaac3d230][0xaaaaaac3ff50] [0xaaaaaac48140][0xaaaaaac48950][0xaaaaaac49160] [0xaaaaaac3b710][0xaaaaaac3f770][0xaaaaaac3e390] [0xaaaaaac47b00][0xaaaaaac47e20][(nil)]
2025-11-17 12:12:50 DEBUG KEY FREE PTR [(nil)][(nil)][(nil)][(nil)] [(nil)][(nil)][(nil)] [(nil)][(nil)][(nil)] [(nil)][(nil)][(nil)]
2025-11-17 12:12:50 DEBUG KEY FREE PTR [0xaaaaaac527e0][0xaaaaaac400b0][0xaaaaaac56440][0xaaaaaac56500] [0xaaaaaac56d80][0xaaaaaac57590][0xaaaaaac57da0] [0xaaaaaac56d20][0xaaaaaac56d50][0xaaaaaac565f0] [0xaaaaaac566e0][0xaaaaaac56a00][(nil)]
2025-11-17 12:12:50 DEBUG KEY FREE PTR [(nil)][(nil)][(nil)][(nil)] [(nil)][(nil)][(nil)] [(nil)][(nil)][(nil)] [(nil)][(nil)][(nil)]

After:

2025-11-17 12:13:53 DEBUG KEY FREE PTR [0xaaaaaac44270][0xaaaaaac3ffe0][0xaaaaaac3d230][0xaaaaaac3ff50] [0xaaaaaac48140][0xaaaaaac48950][0xaaaaaac49160] [0xaaaaaac3b710][0xaaaaaac3f770][0xaaaaaac3e390] [0xaaaaaac47b00][0xaaaaaac47e20][(nil)]
2025-11-17 12:13:53 DEBUG KEY FREE PTR [0xaaaaaac51e70][0xaaaaaac400b0][0xaaaaaac55ad0][0xaaaaaac55b90] [0xaaaaaac56410][0xaaaaaac56c20][0xaaaaaac57430] [0xaaaaaac563b0][0xaaaaaac563e0][0xaaaaaac55c80] [0xaaaaaac55d70][0xaaaaaac56090][(nil)]
2025-11-17 12:13:53 DEBUG KEY FREE PTR [0xaaaaaac61730][0xaaaaaac5f9a0][0xaaaaaac5fa30][0xaaaaaac5fb20] [0xaaaaaac65600][0xaaaaaac65e10][0xaaaaaac66620] [0xaaaaaac5fd50][0xaaaaaac5fd80][0xaaaaaac5fc60] [0xaaaaaac64fc0][0xaaaaaac652e0][(nil)]
2025-11-17 12:13:53 DEBUG KEY FREE PTR [0xaaaaaac6f330][0xaaaaaac73130][0xaaaaaac731c0][0xaaaaaac732d0] [0xaaaaaac73b70][0xaaaaaac74380][0xaaaaaac74b90] [0xaaaaaac73b10][0xaaaaaac73b40][0xaaaaaac733e0] [0xaaaaaac734d0][0xaaaaaac737f0][(nil)]

@cron2
Copy link
Contributor

cron2 commented Nov 16, 2025

There is a CLEAR(*session); at the beginning of tls_session_init(). This makes sure all pointers are zeroed, and free(0) is explicitly allowed by C standard and used a lot across the code. S_UNDEF is also defined to 0, so this should all not be necessary...?

@cron2 cron2 requested a review from schwabe November 16, 2025 12:45
@stoops stoops changed the title Free Null - Bug Fix Free Null - Call Alignment Nov 17, 2025
@stoops
Copy link
Author

stoops commented Nov 17, 2025

I did not know that free null was part of the C standard and I changed the title from stating that this is a bug fix but I still think this is a good observation and code quality to align the init and free methods so I may just keep this PR for reference sake. I'll updated the description with an example gdb log output captured. Thanks.

@schwabe
Copy link
Contributor

schwabe commented Nov 17, 2025

We did a cleanup in the oppositive direction a few years ago, replace all if(ptr) free(ptr) with just free(ptr). As weird as that might look, that is preferred in C programs.

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.

3 participants