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

Semaphore name needs to be randomized, otherwise clash with other users is unavoidable #56

Closed
peci1 opened this issue Aug 31, 2020 · 9 comments
Labels
bug Something isn't working

Comments

@peci1
Copy link
Contributor

peci1 commented Aug 31, 2020

On some of our cluster nodes, when I run Ignition Blueprint/ign-launch1 (the SubT simulator), I get an error:

[Err] [Manager.cc:300] Error initializing semaphore: Permission denied

That points to

https://github.com/ignitionrobotics/ign-launch/blob/9f4a9298b2f549cd0df66d30f9c5942198593ea5/src/Manager.cc#L298

which can fail in case the semaphore with name

https://github.com/ignitionrobotics/ign-launch/blob/9f4a9298b2f549cd0df66d30f9c5942198593ea5/src/Manager.cc#L50

already exists and is created by another user with permissions that don't allow modification of the semaphore (i.e. 644).

I've checked that on the failing nodes, the semaphore indeed exists and is created by someone else:

$ ls -al /dev/shm/sem.*
-rw-r--r-- 1 otheruser rob 32 Aug 17 22:36 /dev/shm/sem.child_semaphore

In such a case, I can't use Ignition launch at all on that cluster node.

It'd be much better to name the semaphore either randomly, or predictably incorporating the username. Some other apps create semaphores named e.g.:

-rw-------. 1 user group 32 Aug 27 17:42 /dev/shm/sem.loky-447964-ctehrdwd
-rw-------. 1 user group 32 Aug 27 17:42 /dev/shm/sem.loky-447964-dfqrdehz
-rw-------. 1 user group 32 Aug 27 17:42 /dev/shm/sem.loky-447964-ed6_sluy
-rw-------. 1 user group 32 Nov 20  2019 /dev/shm/sem.mp-29fggu5g
-rw-------. 1 user group 32 Dec 16  2019 /dev/shm/sem.mp-2croga40
@peci1
Copy link
Contributor Author

peci1 commented Aug 31, 2020

FYI @mjcarroll added the semaphore stuff according to git blame.

@mjcarroll
Copy link
Contributor

Busted :)

I think having a randomized suffix makes perfect sense. Is this something you are interested in contributing?

@peci1
Copy link
Contributor Author

peci1 commented Sep 1, 2020

I can definitely prepare a PR appending a random string/number to the semaphore name. But as I don't know the architecture of ign-launch, I can't foresee the consequences (if there are any).

@mjcarroll
Copy link
Contributor

The semaphore was added to fix the way that we were originally using SIGCHLD. The commit of interest is here: 8ecae13

It was originally supposed to be an unnamed semaphore, but MacOS explicitly has to be named. Because the original implementation was unnamed, I don't see any issue with adding a randomized string. The semaphore is not being accessed across multiple launch instances or being used in a way that the name is needed.

@mjcarroll mjcarroll added the bug Something isn't working label Sep 1, 2020
@peci1
Copy link
Contributor Author

peci1 commented Sep 1, 2020

Okay, then I can prepare the PR. One thing to think through - shouldn't we go with unnamed semaphores on non-Mac platforms?

@mjcarroll
Copy link
Contributor

I opted to used named semaphores for all platforms to prevent having to maintain platform specific code.

peci1 added a commit to peci1/ign-launch that referenced this issue Sep 2, 2020
@peci1
Copy link
Contributor Author

peci1 commented Sep 2, 2020

Fix posted in #57.

peci1 added a commit to peci1/ign-launch that referenced this issue Sep 2, 2020
peci1 added a commit to peci1/ign-launch that referenced this issue Sep 2, 2020
peci1 added a commit to peci1/ign-launch that referenced this issue Sep 2, 2020
mjcarroll pushed a commit that referenced this issue Sep 21, 2020
@peci1
Copy link
Contributor Author

peci1 commented Sep 25, 2020

Could I ask for a release of ign-launch1 which includes this fix? It'd simplify our testing...

@peci1 peci1 closed this as completed Sep 25, 2020
@chapulina
Copy link
Contributor

Could I ask for a release

See #61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants