Opened 5 years ago

Last modified 5 years ago

#4495 new defect

Fix closing oscam in *BSD

Reported by: leecher2 Owned by:
Priority: minor Component: General
Severity: medium Keywords: openbsd, freebsd, exit, close, ctrl-c, shutdown
Cc: Sensitive: no

Description

Revision

11273

Issue Description

At least on OpenBSD 5.9,6.0-curent and FreeBSD 10.3-RELEASE, oscam won't exit properly. You must send a kill -9 to terminate the process.

This is due cs_exit() code hitting (!cl) condition. Apparently on OpenBSD and FreeBSD, cur_client() isn't populated at that point, and causes oscam to not terminate under any proper condition. Seems to be a libpthread behavior/issue/feature.

Attached patch fixes for me, and makes the following exit triggers to work again:

-Ctrl-C in terminal when process is in FG (SIGINT)
-Normal "kill" (SIGTERM)
-Web shutdown/resart
-start/stop rc.d script (apparently does SIGTERM too).

Please take this ticket as a request for comments, as I'm not involved in oscam code and don't know if bypassing (!cl) code may trigger anything worse than this bug itself. Also, this code is untested on Linux.

When the issue occurs

Trying to exit oscam on OpenBSD and FreeBSD

Attachments (3)

fix-close-bsd.diff (744 bytes ) - added by leecher2 5 years ago.
fix-close-bsd-v2.diff (519 bytes ) - added by leecher2 5 years ago.
fix-close-bsd-v3.diff (481 bytes ) - added by leecher2 5 years ago.

Download all attachments as: .zip

Change History (17)

by leecher2, 5 years ago

Attachment: fix-close-bsd.diff added

by leecher2, 5 years ago

Attachment: fix-close-bsd-v2.diff added

comment:1 by leecher2, 5 years ago

please don't mind first attachment, has some testing rubbish. Correct is v2.

comment:2 by khimtiki, 5 years ago

Well, in my rc.d script I use "kill -9 oscam" for many years, so I didn't notice such problem. Your patch should be tested more complex on different systems... Did you test it on any Linux? maybe you'll be able to check it in some kind of virtual environment?

in reply to:  2 comment:3 by leecher2, 5 years ago

Replying to khimtiki:

Well, in my rc.d script I use "kill -9 oscam" for many years, so I didn't notice such problem. Your patch should be tested more complex on different systems... Did you test it on any Linux? maybe you'll be able to check it in some kind of virtual environment?

kill -9 is not a clean shutdown, you have to do it because oscam is trapping catchable signals to do a clean shutdown, and the clean shutdown routine is broken for BSD. Sure the kernel will terminate all connections and deallocate al objects, but that's not the clean way. Have you tested the patch?

It also makes easier to test oscam setup in debug mode (foreground) without the need of killing it from another session or sending it to background and killing it.

In the meantime, I also tested patch on Linux and OS X, both working fine with it. OSX apparently does not have the BSD problem and works also fine without the patch, thats interesting because I would have sweared it would share pthreads code from FreeBSD. I spotted some other bugs related to BSD in the way, and I'm working on fixes.

comment:4 by svk900, 5 years ago

This patch doesn't work for openbsd 5.9 x86.

in reply to:  4 ; comment:5 by leecher2, 5 years ago

Replying to svk900:

This patch doesn't work for openbsd 5.9 x86.

Just tested for you on OpenBSD 5.9 i386 and it works

Attaching screenshot, you can see that hitting Ctrl-C triggers a clean exit. Did you applied the patch correctly and are executing the right file?

EDIT: apparently trac rejects screenshot upload, I provide a link to same picture instead: http://oi68.tinypic.com/2wdc5yv.jpg

Last edited 5 years ago by leecher2 (previous) (diff)

in reply to:  5 ; comment:6 by svk900, 5 years ago

Replying to leecher2:

Replying to svk900:

This patch doesn't work for openbsd 5.9 x86.

Just tested for you on OpenBSD 5.9 i386 and it works

Attaching screenshot, you can see that hitting Ctrl-C triggers a clean exit. Did you applied the patch correctly and are executing the right file?

EDIT: apparently trac rejects screenshot upload, I provide a link to same picture instead: http://oi68.tinypic.com/2wdc5yv.jpg

I applied the patch again but it doesn't work for me.
Ctrl-C and kill pid number don't work.
The only way to kill oscam is kill -9 or by using the webinterface and click shutdown.

in reply to:  6 comment:7 by leecher2, 5 years ago

Replying to svk900:

I applied the patch again but it doesn't work for me.
Ctrl-C and kill pid number don't work.
The only way to kill oscam is kill -9 or by using the webinterface and click shutdown.


That's unusual since the patch should be system and processor independent.

Please run the following commands in your oscam source tree and send me the buffer of your terminal when you're done:

grep -A 30 "void cs_exit("  oscam.c
gmake distclean
gmake 2>/dev/null
Distribution/oscam-1.20-*.debug -c /tmp

(hit ctrl-c when oscam is running, and wait 5 seconds)

Thank you
Regards

comment:8 by svk900, 5 years ago

I find out this patch is working as long as i don't use a oscam.conf file.
When i use a config file i'm unable to kill or ctrl-c oscam.

void cs_exit(int32_t sig)
{
        if(cs_dump_stack && (sig == SIGSEGV || sig == SIGBUS || sig == SIGQUIT))
                { cs_dumpstack(sig); }

        set_signal_handler(SIGHUP , 1, SIG_IGN);
        set_signal_handler(SIGPIPE, 1, SIG_IGN);

        struct s_client *cl = cur_client();
        if(!cl)
                { goto exit; }

        // this is very important - do not remove
        if(cl->typ != 's')
        {
                cs_log_dbg(D_TRACE, "thread %8lX ended!", (unsigned long)pthread_self());

                free_client(cl);

                //Restore signals before exiting thread
                set_signal_handler(SIGPIPE , 0, cs_sigpipe);
                set_signal_handler(SIGHUP  , 1, cs_reload_config);

                pthread_exit(NULL);
                return;
        }

exit: if(!exit_oscam)
                { exit_oscam = sig ? sig : 1; }
}

in reply to:  8 comment:9 by leecher2, 5 years ago

Replying to svk900:

I find out this patch is working as long as i don't use a oscam.conf file.
When i use a config file i'm unable to kill or ctrl-c oscam.


It may sound silly, but are you positive you replaced your oscam executable file with the patched build? The one your startup script runs, usually on /usr/local/bin. This simple command may tell:

find / -name "oscam*" -perm -550 -type f -exec ls -l {} \; -exec md5 {} \; 2>/dev/null

If so, what kind of environment do you have? Any physical card reader? It may be helpful to take a look to your config files, can you strip them off passwords and hostnames, pack them, and attach to the ticket?

I have it working on my setup with full configuration, plenty of readers and users. Your issue is very strange but it's worth to take a deep look at it.

comment:10 by svk900, 5 years ago

I created a different patch file and now it's working perfect.
The patch is doing the same.
Did you build oscam on OpenBSD?

--- oscam.c     Sun Aug 28 13:46:50 2016
+++ oscam.c     Sun Aug 28 13:46:41 2016
@@ -759,7 +759,7 @@

        struct s_client *cl = cur_client();
        if(!cl)
-               { return; }
+               {

        // this is very important - do not remove
        if(cl->typ != 's')
@@ -775,6 +775,7 @@
                pthread_exit(NULL);
                return;
        }
+               }

        if(!exit_oscam)
                { exit_oscam = sig ? sig : 1; }

comment:11 by nm1, 5 years ago

The code is wrong.

in reply to:  10 ; comment:12 by leecher2, 5 years ago

Replying to svk900:

I created a different patch file and now it's working perfect.
The patch is doing the same.
Did you build oscam on OpenBSD?


That patch is wrong.

First you're cheking that there's no cl, but then you check for cl->typ value, At that point cl is a null pointer and will trigger a SIGSEGV (Segmentation Fault). It will cause a memory protection fault, not a clean exit.

If the goto code is the culprit for you, please test with this patch:

--- oscam.c     Tue Aug 23 19:43:15 2016
+++ oscam.c     Sun Aug 28 14:27:28 2016
@@ -758,11 +758,9 @@ void cs_exit(int32_t sig)
        set_signal_handler(SIGPIPE, 1, SIG_IGN);
 
        struct s_client *cl = cur_client();
-       if(!cl)
-               { return; }
 
        // this is very important - do not remove
-       if(cl->typ != 's')
+       if(cl && cl->typ != 's')
        {
                cs_log_dbg(D_TRACE, "thread %8lX ended!", (unsigned long)pthread_self());

in reply to:  12 ; comment:13 by svk900, 5 years ago

That patch is wrong.

First you're cheking that there's no cl, but then you check for cl->typ value, At that point cl is a null pointer and will trigger a SIGSEGV (Segmentation Fault). It will cause a memory protection fault, not a clean exit.

If the goto code is the culprit for you, please test with this patch:

--- oscam.c     Tue Aug 23 19:43:15 2016
+++ oscam.c     Sun Aug 28 14:27:28 2016
@@ -758,11 +758,9 @@ void cs_exit(int32_t sig)
        set_signal_handler(SIGPIPE, 1, SIG_IGN);
 
        struct s_client *cl = cur_client();
-       if(!cl)
-               { return; }
 
        // this is very important - do not remove
-       if(cl->typ != 's')
+       if(cl && cl->typ != 's')
        {
                cs_log_dbg(D_TRACE, "thread %8lX ended!", (unsigned long)pthread_self());

Thx this works too!

in reply to:  13 comment:14 by leecher2, 5 years ago

Replying to svk900:

Thx this works too!


Thanks for you feedback.

I'm uploading this as v3 of the patch; it simpler and removes some code, which is always better.

by leecher2, 5 years ago

Attachment: fix-close-bsd-v3.diff added
Note: See TracTickets for help on using tickets.