xrun: Fix potential concurrency issues #67
Conversation
xrun/src/xrun.c
Outdated
| struct container *container = NULL; | ||
|
|
||
| k_mutex_lock(&container_lock, K_FOREVER); | ||
| container = find_container_locked(container_id); |
There was a problem hiding this comment.
This is prone to ABA problem. Reference owner should hold own pointer to the container.
xrun/src/xrun.c
Outdated
| return ret; | ||
| } | ||
|
|
||
| put_container(container); | ||
| return unregister_container_id(container_id); |
There was a problem hiding this comment.
You can just call put_container for a second time here. With a comment, of course.
xrun/src/xrun.c
Outdated
| ret = domain_unpause(container->domid); | ||
| if (ret) { | ||
| put_container(container); | ||
| return ret; | ||
| } | ||
|
|
||
| container->status = RUNNING; | ||
| put_container(container); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
| ret = domain_unpause(container->domid); | |
| if (ret) { | |
| put_container(container); | |
| return ret; | |
| } | |
| container->status = RUNNING; | |
| put_container(container); | |
| return 0; | |
| } | |
| ret = domain_unpause(container->domid); | |
| if (ret) { | |
| goto out; | |
| } | |
| container->status = RUNNING; | |
| out: | |
| put_container(container); | |
| return ret; | |
| } |
There was a problem hiding this comment.
Not mandatory. Just will look more readable from my standpoint. The same suggestion across all put_container calls
Deedone
force-pushed
the
container_refcount
branch
from
d72414c to
960d7e5
Compare
Deedone
changed the title
|
Similar comment as #72 (comment) |
| * - Add reference counting (like linux kref) to | ||
| * ensure that this object does not disappear under | ||
| * your feet. | ||
| */ |
There was a problem hiding this comment.
It is a pity, but this function has another concurrency issue: code below checks if container exists by using get_container. Problem is that if register_container_id is called twice with the same container id, it may register both containers. You need to hold the container lock whole time: obtain the lock, check if container already exists, create new one, free the lock.
This is another issue, so you can add TODO for now. Or make a separate patch that fixes this.
| } | ||
|
|
||
| return unregister_container_id(container_id); | ||
| /* Put container twice to drop the last reference */ | ||
| put_container(container); |
There was a problem hiding this comment.
Hm... should you call domain_destroy when you are freeing container? Or it will be okay to have a container object without alive domain inside?
Deedone
force-pushed
the
container_refcount
branch
from
20879d6 to
9b61505
Compare
|
Oh, and don't forget to fix checkpatch. |
Deedone
force-pushed
the
container_refcount
branch
from
9b61505 to
2078370
Compare
|
Tested with unit tests and trying to start/stop/pause/unpause the aos monkey container. |
Add reference counting to container objects to prevent unexpected failures when container object is freed right after return from get_container function. Signed-off-by: Mykyta Poturai <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]> Reviewed-by: Dmytro Firsov <[email protected]>
Add locking to pause and resume operations to prevent container->status from getting out of sync with the actual state of the container. Signed-off-by: Mykyta Poturai <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]> Reviewed-by: Dmytro Firsov <[email protected]>
Deedone
force-pushed
the
container_refcount
branch
from
2078370 to
fe9f3e7
Compare
Add reference counting to container objects to prevent unexpected failures when container object is freed right after return from get_container function.
Add locking to pause/resume functions.