WIP: signal -> sigaction

[nsajko]

The signal function is underspecified and deprecated in favor of sigaction, make the replacement.

Also added includes wheremissing, and fixed a "_UNIX" to "UNIX".

[radare]

imho is better to have this implemented in a custom function in r_sys (libr/util/sys.c) and just have the ifdef for UNIX in one place, and make 3 lines in 1 like it was before

On 31 Aug 2018, at 17:58, Neven Sajko notifications@github.com wrote:

@nsajko commented on this pull request.

In binr/preload/libr2.c https://github.com/radare/radare2/pull/11335#discussion_r214400362:

@@ -37,8 +41,15 @@ static void sigusr2(int s) { static void _libwrap_init() attribute ((constructor)); static void _libwrap_init() { char *web;

• signal (SIGUSR1, sigusr1);
• signal (SIGUSR2, sigusr2);
• struct sigaction sigact;
• sigact.sa_handler = sigusr1;
• sigemptyset(&sigact.sa_mask); will fix

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/radare/radare2/pull/11335#discussion_r214400362, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3-lvQrI_JWLjDN_9oMZRWZF6kH7w_vks5uWV0zgaJpZM4WVcm1.

[nsajko]

Would the function take as arguments the signal and handler, or also the signal mask?

[radare]

not sure if we want the mask .. i would probably make the mask an internal thing of that function

On 31 Aug 2018, at 18:03, Neven Sajko notifications@github.com wrote:

Would the function take as arguments the signal and handler, or also the signal mask?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/radare/radare2/pull/11335#issuecomment-417710973, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3-lv1DDt7eYp4a_WlICJVhjpU2SxS8ks5uWV5OgaJpZM4WVcm1.

[nsajko]

So the mask would always contain just the one signal?

[radare]

we can have this as static inside the function, so it wouldnt be always set until you call it with -1 in the signal number for example. this way we dont need to pass more arguments or hold that variable across multiple calls

On 31 Aug 2018, at 18:16, Neven Sajko notifications@github.com wrote:

So the mask would always contain just the one signal?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/radare/radare2/pull/11335#issuecomment-417714695, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3-lk2UN-2AqEGDYyRW5QfbF61yckeXks5uWWFXgaJpZM4WVcm1.

[nsajko]

But still we could not set the mask to multiple signals, for example we might want to set the mask to block SIGINT, SIGHUP, and SIGBUS; or SIGUSR1 and SIGUSR2.

[nsajko]

I mean, it seems to me that having a custom function only makes sense if you are only ever going to have up to one signal in the mask. So we could not use the custom function in all of the places where we install signal handlers.

[radare]

as reference: https://stackoverflow.com/a/232711

[nsajko]

@radare What do you think about my last two comments? To restate the issue, we sometimes need to block more than one signal in signal handlers, thus in those cases we have to use sigaction directly, so why have a custom function?

[radare]

We can solve that with vararg and a single function because i never liked how sigaction works but its clearly better than signal

On 2 Sep 2018, at 13:20, Neven Sajko notifications@github.com wrote:

@radare What do you think about my last two comments? To restate the issue, we sometimes need to block more than one signal in signal handlers, thus in those cases we have to use sigaction directly, so why have a custom function?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[radare]

can you address the comments?

[codecov-io]

Codecov Report

Merging #11335 into master will increase coverage by 0.42%. The diff coverage is 35.55%.

@@            Coverage Diff             @@
##           master   #11335      +/-   ##
==========================================
+ Coverage   37.07%   37.49%   +0.42%     
==========================================
  Files         903      899       -4     
  Lines      288562   284178    -4384     
==========================================
- Hits       106974   106561     -413     
+ Misses     181588   177617    -3971

Impacted Files	Coverage Δ
binr/rarun2/rarun2.c	23.4% <0%> (-1.05%)	:arrow_down:
libr/core/rtr.c	1.41% <0%> (-0.01%)	:arrow_down:
libr/io/p/io_tcp.c	2.04% <0%> (-0.03%)	:arrow_down:
libr/socket/socket.c	0% <0%> (ø)	:arrow_up:
libr/io/p/io_self.c	1.04% <0%> (-0.02%)	:arrow_down:
libr/core/cmd.c	47.4% <100%> (ø)	:arrow_up:
libr/util/sys.c	54.62% <53.84%> (+1.88%)	:arrow_up:
libr/cons/cons.c	57.14% <77.77%> (+0.13%)	:arrow_up:
libr/util/assert.c	0% <0%> (-66.67%)	:arrow_down:
libr/util/log.c	0% <0%> (-25.93%)	:arrow_down:
... and 113 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 218cdd7...c82d9a4. Read the comment docs.

[nsajko]

I am not sure that variadic arguments are a good solution, because sometimes we may need a complement of a set of signals for the mask, which would necessitate adding another argument to the API of r_sys_sigaction and complicate the implementation, at which point we are better of just directly passing a sigset_t argument for the mask.

[nsajko]

ping @radare

[ret2libc]

With this branch, if you are in Visual Mode and you change the screen size, it brings you back to "regular mode". This does not happen on master.

[ret2libc]

With this branch, if you are in Visual Mode and you change the screen size, it brings you back to "regular mode". This does not happen on master.

Moreover, after this happens, ^D doesn't work anymore to exit radare2.

[nsajko]

With this branch, if you are in Visual Mode and you change the screen size, it brings you back to "regular mode". This does not happen on master.

I think this is because in master screen refreshes because of SIGWINCH don't happen at all until some input comes, presumably because of SA_RESTART. So this is just concealing a preexisting bug...

Moreover, after this happens, ^D doesn't work anymore to exit radare2.

This is also the case on master ...

[ret2libc]

Yeah, I later noticed it was happening on master too, but only recently I think. Anyway, let's try to fix that since it's related to signals AFAIK.

[nsajko]

@radare, I do not see how to further improve this commit; that is, it seems fine to me. You wanted me to move the preprocessor #ifs to inside the body of r_sys_sigaction, but the problem is that would still need some ifdefing for sigset_t because it is a parameter of r_sys_sigaction. In fact, all the other #ifs added in this commit are because of sigset_t.

[ret2libc]

My opinion is that it doesn't have a lot of sense to create r_sys_sigaction if it's only callable from UNIX.

The idea should be to have platform-independent argument types so that the function can be called in all cases (maybe in some cases, e.g. Windows, it won't do anything, but it should still be callable). For example..

#if windows
#define r_sigset_t u32
#else
#define r_sigset_t sigset_t
#endif

R_API int r_sigemptyset(r_sigset_t *set) {
#if __UNIX__
    return sigemptyset(set);
#else
    return 0;
#endif
}

R_API int r_sys_sigaction(int sig, r_sighandler_t cb, r_sigset_t *mask, int flags) {
#if __UNIX__
    ....
#else
    return 0;
#endif
}

In this way, I think, you should be able to call both functions and use the types without issues on both platforms that do support signals and those that don't. The compiler should be smart enough to remove useless calls (maybe you have to put some definitions in the .h file for this).

[radare]

Thats what i was trying to say. This function can implement it using signal if sigaction is not found. On windows.. thats another thing but we can do something i guess

On 13 Sep 2018, at 08:10, Riccardo Schirone notifications@github.com wrote:

My opinion is that it doesn't have a lot of sense to create r_sys_sigaction if it's only callable from UNIX.

The idea should be to have platform-independent argument types so that the function can be called in all cases (maybe in some cases, e.g. Windows, it won't do anything, but it should still be callable). For example..

if windows

define r_sigset_t u32

else

define r_sigset_t sigset_t

endif

R_API int r_sigemptyset(r_sigset_t *set) {

if UNIX

return sigemptyset(set);

else

return 0;

endif

}

R_API int r_sys_sigaction(int sig, r_sighandler_t cb, r_sigset_t *mask, int flags) {

if UNIX

....

else

return 0;

endif

} In this way, I think, you should be able to call both functions and use the types without issues on both platforms that do support signals and those that don't. The compiler should be smart enough to remove useless calls (maybe you have to put some definitions in the .h file for this).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[nsajko]

OK, guys. I thought the #if __WINDOWS__ #define sigset_t would be ugly, but if you guys want it i'll try to do something like that.

This function can implement it using signal if sigaction is not found. On windows.. thats another thing but we can do something i guess

Windows only has SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM; with SIGINT unsupported on Win32. Thus it is maybe better to just do nothing on Windows.

[ret2libc]

Why is that ugly? It's the way to go, IMHO, instead of spreading #if all around. If Windows 11 next year will support sigset_t the same way as linux, you won't need to touch 10000 files/lines, but just a couple of places and everything will work.

[nsajko]

Another problem: we will need to also #define all signals we use that are not in the standard, like SIGALRM, SIGWINCH, SIGKILL, because those macros will not be defined on Windows.

[ret2libc]

Another problem: we will need to also #define all signals we use that are not in the standard, like SIGALRM, SIGWINCH, SIGKILL, because those macros will not be defined on Windows.

True, that may be a problem. Actually... after looking at this more, why do we even want to have a r_sys_sigaction function? @radare

Most of the places where this is used are already under #if UNIX. So... is it really worth? I'm no expert of windows, but can we assume things will work well there? Either we abstract things a lot, or at this point we could just directly use sigaction...

[radare]

my idea of having r_sys_signation was to solve the following problems:

• avoid extra #if UNIX
• avoid to use sigset_t and sigemptyset ..
• simplify the use

        sigset_t mask;
    sigemptyset (&mask);
    r_sys_sigaction (SIGUSR1, sigusr1, &mask, 0);
    r_sys_sigaction (SIGUSR2, sigusr2, &mask, 0);

So imho we should implement this with vargarg function like this:

r_sys_sigaction (SIGUSR1, sigusr1, SIGUSR2, sigusr2, -1);

do this makes sense to you?

[ret2libc]

@radare I got what you wanted, but it's also true as @nsajko said that you'd need to redefine all SIG* macros for windows, etc.

[radare]

yeah, but we have r_signal.h which may be used as a generic definition for most common signals across OS

On 17 Sep 2018, at 12:53, Riccardo Schirone notifications@github.com wrote:

@radare https://github.com/radare I got what you wanted, but it's also true as @nsajko https://github.com/nsajko said that you'd need to redefine all SIG* macros for windows, etc.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/radare/radare2/pull/11335#issuecomment-421966791, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3-ll4i1bz9XmBa5yqoklFfL-4XOjvGks5ub38qgaJpZM4WVcm1.

[nsajko]

r_sys_sigaction (SIGUSR1, sigusr1, SIGUSR2, sigusr2, -1);

You are forgetting that choosing the signal mask is separate from choosing for which signal to install a handler (that is among other things why sigaction needs to replace signal). So the call would need to be something like:

r_sigset sigmask1, sigmask2;
r_sigemptyset(&sigmask1);
r_sigfullset(&sigmask2);
r_sigaddset(&sigmask1, SIGXXX);
r_sigaddset(&sigmask1, SIGXXX);
r_sigdelset(&sigmask2, SIGXXX);
r_sys_sigaction (SIGUSR1, sigusr1, sigmask1, SIGUSR2, sigusr2, sigmask2, -1);

But that implies a lot of work (defining all the signal numbers, r_sigset, r_sigemptyset, r_sigfullset, r_sigaddset, r_sigdelset, r_sys_sigaction) for a benefit that is not clear, because POSIX is the only platform that has full signal support, and all the places where we want to use r_sys_sigaction are already under #if UNIX.

[ret2libc]

I do agree with @nsajko . Though I also initially thought as @radare , after thinking about it a bit more I think there's no real value in creating all those r_signal functions. Signals are a unix concept, so I don't think it's really worth the extra engineering to have platform-independent functions/macros.

And as said, all sigaction uses are already under #if UNIX

[radare]

Any example?

On 21 Sep 2018, at 01:04, Neven Sajko notifications@github.com wrote:

@nsajko commented on this pull request.

In libr/socket/socket.c:

@@ -731,7 +743,9 @@ R_API char r_socket_to_string(RSocket s) { R_API int r_socket_write(RSocket s, void buf, int len) { int ret, delta = 0;

if UNIX || defined(CYGWIN)

• signal (SIGPIPE, SIG_IGN);
• sigset_t mask;
• sigemptyset (&mask);
• r_sys_sigaction (SIGPIPE, SIG_IGN, &mask, 0); Why would it return the signal mask, we are not gonna need it.

Please read my last comment:

You are forgetting that choosing the signal mask is separate from choosing for which signal to install a handler (that is among other things why sigaction needs to replace signal).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[nsajko]

Any example?

For example of where we may want a different signal mask: in libr/util/sys.c where sigaction is already being used probably it would be best to just initialize the signal mask with sigfullset and not delete any signals, instead of putting just all the signals that we are installing handlers for in the mask.

[radare]

I still dont get that usecase, is this happening in any place in the code?

About the 0 flag. from what i read in the mac manpage i understand that it shuold never be 0 because thats deprecated and wrong. so i think is better to just not pass that argument to the function and handle it inside our r_sys_sigaction()

The traditional BSD style is not portable and since its capabilities are a full subset of a SA_SIGINFO handler, its use is deprecated.

about the generic r_signal api, we should unify all the code that is in debug, util and such to get a single place to resolve signal by name/by number as well as having r_signal_action() (instead of r_sys_sigaction).

but we can do that later in a different pr

On 21 Sep 2018, at 11:07, Neven Sajko notifications@github.com wrote:

Any example?

For example of where we may want a different signal mask: in libr/util/sys.c where sigaction is already being used probably it would be best to just initialize the signal mask with sigfullset and not delete any signals, instead of putting just all the signals that we are installing handlers for in the mask.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/radare/radare2/pull/11335#issuecomment-423466994, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3-lgbulNlqAX2dQ7U0ftWovqe4cpFLks5udKxRgaJpZM4WVcm1.

[nsajko]

I still dont get that usecase, is this happening in any place in the code?

in libr/util/sys.c where sigaction is already being used ...

About the 0 flag. from what i read in the mac manpage i understand that it shuold never be 0 because thats deprecated and wrong.

We can change to SA_SIGINFO later, that would require changing the signal handlers. But, I do not see any benefit in that.

[nsajko]

The commit "libr/util/sys.c: Block all signals in the crash signal handler" should clear up why the mask parameter is needed.

[nsajko]

Is this OK now?

[radare]

I dont like having an rsys qrapper of a function that gives no value to it because it still requires using sig* stuff before calling it so it doesnt makes the code smaller or more portable. Imho all the logic must go inside

On 30 Sep 2018, at 19:46, Neven Sajko notifications@github.com wrote:

Is this OK now?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[radare]

ping

[radare]

hey!

[radare]

ping

[radare]

i'll take care of this after release. tired of waiting