Page MenuHomeFreeBSD

Fix some leaks in usr.sbin/bhyve/block_if.c
Needs ReviewPublic

Authored by slw_zxy.spb.ru on Fri, Jun 26, 2:31 PM.
Tags
None
Referenced Files
F161559168: D57887.diff
Sat, Jul 4, 8:59 PM
Unknown Object (File)
Sat, Jul 4, 12:25 PM
Unknown Object (File)
Sat, Jul 4, 12:21 PM
Unknown Object (File)
Sat, Jul 4, 4:37 AM
Unknown Object (File)
Fri, Jul 3, 8:20 AM
Unknown Object (File)
Thu, Jul 2, 11:28 AM
Unknown Object (File)
Tue, Jun 30, 2:32 PM
Unknown Object (File)
Tue, Jun 30, 12:59 AM

Details

Reviewers
jhb
Group Reviewers
bhyve
Summary

Some resource allocated and not correctly freed in usr.sbin/bhyve/block_if.c.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

novel added inline comments.
usr.sbin/bhyve/block_if.c
682

What are the scenarios where we end up in the err: label with the bc already allocated? Last time we jump to the err label is when bc allocation fails, so still nothing to clean up at this point, and then we don't seem to end up there at all.

870

I think style(9) says to declare variables before statements.

usr.sbin/bhyve/block_if.c
682

In internall repo we have additional init before creating blockif_thr threads.

870

I see, will be fixed.

bnovkov added inline comments.
usr.sbin/bhyve/block_if.c
683–685

Unfortunately we have no way of knowing whether these were successfully created since we don't check for the return values of pthread_{mutex,cond}_* calls above. Could you please add those checks and adjust the cleanup code accordingly?

glebius added inline comments.
usr.sbin/bhyve/block_if.c
682

Without your internal changes the diff looks strange. Why not to free resources right in the block where if (bc == NULL) {? In case you are not going to upstream your other changes, the best way would be to make the upstreamed change styled correctly: destroy resources right after calloc() failure, then refactor your internal changes upon that. For example you can goto up the code to fall into the beginning of the if (bc == NULL) { block.

683–685

And pthread_create, which is way more likely to fail.

NB: This function may benefit from __attribute__((cleanup)). I don't know if we already have a precedence of using it in src.

Check pthread_create/pthread_mutex_init/pthread_mutex_init

The patch is updated to match all your comments. What needs to be done from my side to get it pushed to main?