fix core cause of openfortivpn is locked when spawning ppp is failed.

[Mabin-J]

This commit is related with #132 and 158f489 .

It seems that pthread_cancel cannot effect to dispatch_semaphore_wait.
(dispatch_semaphore_wait is used for sem_wait for macOS, io.c#L41)
It seems that dispatch_semaphore_wait not use real semaphore.

If pthread_cancel cannot effect to dispatch_semaphore_wait,
if_config should be returned at sem_wait(&sem_if_config);(io.c#L509) but it isn't...
openfortivpn is locked that line.

#132 is not best solution because when run sem_post(&sem_if_config);(io.c#L299), io.c#L510-L534 will be ran.

In Linux, when run pthread_cancel(if_config_thread);(io.c#L609),
sem_wait(&sem_if_config);(io.c#L509) will be broken and if_config function will be returned immediately.

So I add code for breaking after sem_wait when sem_post(&sem_stop_io); is called.

I think that remain this logic before find alternative dispatch_semaphore.

[Mabin-J]

This PR's purpose is same with 158f489 so if possible, I want to merge 158f489.

I'm so sorry I make you to check twice same issue.

[adrienverge]

Hi @Mabin-J,

Thanks for your help on making openfortivpn MacOS-compatible, it is appreciated!

However I don't understand the goal of your latest pull-requests. Could you open a GitHub issue to clearly state:

1. what are the current problems on MacOS,
2. your vision on how to fix them (for example by describing a set of pull-requests to merge).

This way @mrbaseman, @DimitriPapadopoulos and I can follow your contributions efficiently and merge them while avoiding regressions and bugs.

Also, please make sure your commits are clear (e.g. not io.c: change code to solve #132), documented if possible, atomic and revertable. This way in case of a bug, we can easily find the cause and a nice solution.

Thanks!

[Mabin-J]

Hi @adrienverge.

I'll do it.
Thanks for guide.

[mrbaseman]

Hello @Mabin-J,

I would like to understand what exactly this patch does, and I believe I did, but please correct me if I got something wrong. If I understand your code correctly, you do 2 things:

• you revert fix lock status when fail to spawn pppd in macOS. #132 because it did not work correctly. It allowed to kill openfortivpn with Ctrl + C but pppd still went in zombie state, right?
• now you have found the following solution to avoid this zombie state: If I understand it right pthread_cancel does not cancel the threads on MacOS X and you work around this by setting is_destroy and calling sem_post which is defined as dispatch_semaphore_signal in the Apple code. This wakes up the hanging threads and the functions which were stuck in sem_wait check is_destroy and immediately exit. Did I get that right?

Apart from reassuring myself that I understood the change correctly, I would suggest to rename is_destroy to some a bit more specific name like sem_cancel_pthreads (or do you have a better idea?), and there is an unnecessary empty line added (619) which I would like to be removed before merge.

Thanks for your efforts in improving the MacOS X code in openfortivpn.

[Mabin-J]

I checked your comment and #105.
I tested semaphore_* and it was succeed.

But I didn't test in Linux.
I'll test in Linux tomorrow.

[Mabin-J]

Before #132, openfortivpn wasn't finished when I pushed Ctrl + C. it was only finished when I ran kill pid.
And after I ran kill pid, zombie process disappeared.

I think that Zombie process caused locked thread(if_config_thread).
When if_config_tread was finished correctly, zombie process didn't appear.

[Mabin-J]

It works also in Linux correctly. :)

[mrbaseman]

ok, this is a different approach now.

• Does it address GCD semaphores might be unsafe when used in signal handlers #105? could you give a reference, please? In the commit message you have added a link to OpenJDK source, but in GCD semaphores might be unsafe when used in signal handlers #105 in the context of JDK BSD kqueue / kevent are mentioned. I can't see the connection to your commit. Maybe I missed it?

• In the latest version you have introduced capital letter versions of the macros and you had to change the linux code as well. Is this really needed? Wouldn't the following work in the MacOS X code branch?
  #define sem_init(sem, x, value) semaphore_create(mach_task_self(), sem, \
SYNC_POLICY_FIFO, value)
#define sem_wait(sem) semaphore_wait(*sem)
#define sem_post(sem) semaphore_signal(*sem)
#define sem_destroy(sem) semaphore_destroy(mach_task_self(), *sem)
  Maybe you tried that already but if this is not possible I would like to understand why.

[Mabin-J]

• I think that using kqueue/kevent is difficult because we need to make sem_* function with kqueue/kevent function. so I used semaphore_* function like OpenJDK. I think that Oracle still use semaphore_* because it is still useful.
  (http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/file/8b16790cd73a/src/os/bsd/vm/os_bsd.cpp)
  I found that JDK9 still use semaphore_* (http://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/1ae9e84f68b3/src/os/bsd/vm/os_bsd.cpp)

• When I used #define sem_*(sem) semaphore_*(*sem), It didn't works. I think that because sem_* functions already exist in macOS and these didn't work. it means, sem_* cannot be overrided. I think that maybe this reason, OpenJDK also used SEM_* define.

[mrbaseman]

Ah, now I see, on L1940 and L1905 ff. of the links you have posted. They do it the same way as you have proposed for openfortivpn

[mrbaseman]

I have just landed #134 on master and resolved the resulting conflict in io.c with 903d091 and 45f5545
@Mabin-J maybe you can squash these commits into your original proposal.

[Mabin-J]

@mrbaseman, I finished.

[mrbaseman]

@Mabin-J thanks. There is still discussion in #105. I'd still wait a bit for comments there.

[mrbaseman]

Dear @Mabin-J

in #105 we have come to the conclusion that this PR is a good step forward, not only to fix the locking issue here, but also as a solution for #105. I would kindly ask you to update the commit message once more, so that it also includes this conclusion. I'd suggest the following:

io.c: fix core cause of openfortivpn is locked when spawning pppd has failed.

This commit reverts the temporary fix from #132 (commit 158f489) and instead replaces GCD semaphores with Mach semaphores, following the lines of http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/file/8b16790cd73a/src/os/bsd/vm/os_bsd.cpp.

By doing so, this commit is also a solution for issue #105 (GCD semaphores might be unsafe when used in signal handlers): On MacOS X named POSIX semaphores, which are known to be async-signal-safe, are implemented upon Mach semaphores (https://books.google.de/books?id=K8vUkpOXhN4C&pg=PA1219&lpg=PA1210&focus=viewport&dq=semaphore_create&hl=de). An analysis of the use of semaphores in openfortivpn has lead to the conclusion that Mach semaphores are just as good as unnamed POSIX semaphores which unfortunately are not implemented in MacOS X.