[Bug 14789] New: Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Summary: Fix signal handler
           Product: Wine
           Version: CVS/GIT
          Platform: All
        OS/Version: All
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: -unknown
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

The function "fprintf()" does not belong to the list of async-signal-safe
functions.
See section "2.4.3 Signal Actions" from the document "2.4 Signal Concepts".
http://opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html#tag_0...

I guess that a different program design will be needed for your function
"do_sigsegv".

I recommend to change the data type for the variable "watchdog" to
"sig_atomic_t".

http://source.winehq.org/git/wine.git?a=blob;f=server/signal.c;h=5e4fe33c6a3...

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Vitaliy Margolen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |WONTFIX

--- Comment #1 from Vitaliy Margolen   2008-08-07 12:10:48 ---
The fprintf there used on an error paths and for debug only.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Markus Elfring  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|WONTFIX                     |

--- Comment #2 from Markus Elfring   2008-08-07 12:53:36 ---
This debug output is implemented by the wrong function.

Would you like to achieve complete async-signal-safety?
http://en.wikipedia.org/wiki/Reentrant

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Vitaliy Margolen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |WONTFIX

--- Comment #3 from Vitaliy Margolen   2008-08-07 15:17:46 ---
Huh?

There is nothing to be fixed here. Closing. Unless you have a patch to send,
don't bother reopening.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Vitaliy Margolen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #4 from Vitaliy Margolen   2008-08-07 15:31:21 ---
First of all wineserver is a single threaded app. So while it processing
signals nothing else works.
Second - that is a pure debug function that is not even used by default - one
have to recompile wineserver with #define DEBUG_OBJECTS 1. And then send -HUP
to the server.
Third it's called from the signal handler which guarantees it won't ever
reenter.

So I don't see what reentrancy are you talking about here?

Closing - there is nothing to fix.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Chris  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #5 from Chris   2008-08-07 23:15:23 ---
> Second - that is a pure debug function that is not even used by default -
> one have to recompile wineserver with #define DEBUG_OBJECTS 1. And then
> send -HUP to the server.

Just because it's only used in the debug path doesn't make it's use any less
invalid. If anything it'll make debugging harder as this can trip and hide the
real problem being tracked.
> Third it's called from the signal handler which guarantees it won't ever
> reenter.
>
> So I don't see what reentrancy are you talking about here?

Reentrancy is when a function is called while another context/frame is also
inside the function. This can very much happen in a single-threaded app. Take
for example:

1> User code runs
2> User code enters fprintf
3> Signal interrupt (user code is paused inside fprintf)
4> Signal handler enters fprintf (!!both user code and signal handler are
inside fprintf!!)
5> Signal handler leaves fprintf
6> Signal handler exits and resumes user code
7> User code continues inside fprintf
8> User code leaves fprintf

IMO, this is a valid bug. You should not be using fprintf inside a signal
handler because it's not gauranteed to handle this situation.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Markus Elfring  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |UNCONFIRMED
         Resolution|WONTFIX                     |

--- Comment #6 from Markus Elfring   2008-08-08 02:04:45 ---
A printf() function call might result in a deadlock during signal handling.
Would you like to use the function "write()" instead?
http://opengroup.org/onlinepubs/009695399/functions/write.html

Are you going to integrate the following update suggestion for improved source
code portability?
diff --git a/server/signal.c b/server/signal.c
index 5e4fe33..dfdfa9b 100644
--- a/server/signal.c
+++ b/server/signal.c
@@ -99,7 +99,7 @@ static struct handler *handler_sigint;
 static struct handler *handler_sigchld;
 static struct handler *handler_sigio;

-static int watchdog;
+static sig_atomic_t watchdog;

/* create a signal handler */
 static struct handler *create_handler( signal_callback callback )

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch, source

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #7 from Markus Elfring   2008-08-10 07:48:11 ---
Created an attachment (id=15367)
 --> (http://bugs.winehq.org/attachment.cgi?id=15367)
Update suggestion to make more functions async-signal-safe

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #8 from Markus Elfring   2008-08-19 08:10:35 ---
Created an attachment (id=15485)
 --> (http://bugs.winehq.org/attachment.cgi?id=15485)
 Update suggestion for async-signal-safe program termination

A few more places are affected.
http://opengroup.org/onlinepubs/009695399/functions/_Exit.html

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |WONTFIX

--- Comment #9 from Alexandre Julliard   2008-08-20 06:06:14 ---
These are totally non-issues, your changes wouldn't make the slightest
difference, all they would do is break things on non-compliant platforms.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #10 from Dmitry Timoshkov   2008-08-20 06:36:03 ---
Closing.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #11 from Markus Elfring   2008-08-20 12:39:57 ---
I would like to point out that the implementation for the function
"segvhandler" is also not async-signal-safe.

I'd prefer more respect for software correctness in this detail.
http://source.winehq.org/git/wine.git?a=blob;f=tools/wmc/wmc.c;h=910a654b3c8...
http://source.winehq.org/git/wine.git/?a=blob;f=tools/wrc/wrc.c;h=15f30a3e7d...

I know that it means a couple of more work to get the affected parts completely
right. Which behaviour do you expect on standard-compliant platforms if unsafe
function calls are used?

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #12 from Alexandre Julliard   2008-08-21 07:38:40 ---
There is no reason at all to worry about that fprintf, first because segv is
actually not an async signal, second because the code aborts right away so we
never even return from the handler, third because if the app is really crashing
inside fprintf not seeing the final error is the least of your worries.

There are more important issues to worry about than the theoretical standard
compliance of a signal handler that never gets triggered anyway.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #13 from Markus Elfring   2008-08-21 08:01:02 ---
I agree that there are other issues with higher importance. Nevertheless, I
would appreciate implementations for signal handlers that are always
asynchronous signal safe.
The current approaches are not guaranteed to work because they depend on
potentially undefined behaviour.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #14 from Alexandre Julliard   2008-08-21 08:14:38 ---
While this may be true in theory, it's completely irrelevant in practice, and
I'm not going to make the code uglier or less portable to solve such a
non-problem.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #15 from Markus Elfring   2008-08-21 08:33:35 ---
It seems to be convenient to ignore consequences by correct programming for
async-signal-safety. The source code portability will be improved if this
particular technical issue will be fixed.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Dan Kegel  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|CVS/GIT                     |unspecified

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #16 from Markus Elfring   2009-03-24 05:12:02 ---
My bug report was another example for the issue "CWE-387: Signal Errors".
http://cwe.mitre.org/data/definitions/387.html

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #17 from Markus Elfring   2009-11-26 04:33:21 ---
Do you agree to the descriptions by Justin Pincar?
https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+o...

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|patch                       |
           Platform|All                         |Other
         OS/Version|All                         |other

--- Comment #18 from Dmitry Timoshkov   2009-11-26 04:49:11 ---
There is nothing to agree or not, because there is nothing to discuss in this
bug report, since there is no any bug. Please re-read all the answers to your
comments, referencing somebody's else comments out of the air won't make your
point of view more valueable.

Again, there is *nothing* to fix.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #19 from Markus Elfring   2009-11-26 05:24:37 ---
(In reply to comment #18)

I propose to fix a specific implementation detail. I find it strange that you
do not admit open issues here.
http://source.winehq.org/git/wine.git/?a=blob;f=server/signal.c;hb=6d1605a6a...
http://source.winehq.org/git/wine.git/?a=blob;f=tools/wmc/wmc.c;hb=1c994d497...
http://source.winehq.org/git/wine.git/?a=blob;f=tools/wrc/wrc.c;h=828176cb74...

I would prefer not to push the topic away just because other issues might be
more challenging.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #20 from Dmitry Timoshkov   2009-11-26 05:28:53 ---
(In reply to comment #19)
> (In reply to comment #18)
> I propose to fix a specific implementation detail.

Once again: there is nothing to fix.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #21 from Markus Elfring   2009-11-26 05:45:44 ---
(In reply to comment #20)

Why do you really want to ignore such a programming error?

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #22 from Dmitry Timoshkov   2009-11-26 11:00:53 ---
(In reply to comment #21)
> Why do you really want to ignore such a programming error?

Repeat after me: there is no error or a bug in the Wine code you are
referring to.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #23 from Markus Elfring   2009-11-26 12:40:09 ---
(In reply to comment #22)

Would you like to understand that it is a clear programming error in the
referred source code places to call a function like "fprintf" (that is not
async-signal-safe) from within signal handler implementations?

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

Dan Kegel  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #24 from Dan Kegel   2009-11-26 20:29:33 ---
Markus, I'm all for safe code, but pick your battles!
As Alexandre said, this code only fires when the server
is crashing, which should never happen.

Could you spend some of your energy helping me triage
buffer overruns (see http://kegel.com/wine/valgrind/logs/ )?

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #25 from Markus Elfring   2009-11-27 05:30:06 ---
(In reply to comment #24)
> Markus, I'm all for safe code, but pick your battles!

It would be easy to use the data type "sig_atomic_t" and to delete the
misplaced fprintf() calls ...
> As Alexandre said, this code only fires when the server
> is crashing, which should never happen.

The source code has got more open issues like "Check return codes everywhere"
that result in different opinions about the involved probabilities.
http://bugs.winehq.org/show_bug.cgi?id=14788
> Could you spend some of your energy helping me triage
> buffer overruns (see http://kegel.com/wine/valgrind/logs/ )?

I assume that some of them can be cleaned up by more object-oriented
programming (in C++).

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #26 from Dan Kegel   2009-11-27 08:37:23 ---
Telling a project that it needs to switch the language it's
written in is generally not a good idea.  *plonk*

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #27 from Markus Elfring   2009-11-27 09:08:36 ---
(In reply to comment #26)

The C programming language makes it inherently easy to "overlook" specific
implementation details (see also bug #14788). So we try to point out open
issues from code reviews by dynamic or static analysis. The willingness for
cooperation of fix integration varies.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #28 from Dan Kegel   2009-11-27 09:27:59 ---
Dude, if you hadn't noticed, we're already paying more attention
to coverity and purify than a lot of projects, and we did it
before you told us how to do our job.  Now please get lost.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #29 from Dmitry Timoshkov   2009-11-27 11:00:12 ---
(In reply to comment #27)
> The C programming language makes it inherently easy to "overlook" specific
> implementation details (see also bug #14788). So we try to point out open
> issues from code reviews by dynamic or static analysis. The willingness for
> cooperation of fix integration varies.

Usually suggestions like above show complete ignorance of the subject.
If you believe that you can't write a trustworthy code - go ahead and
find some other speciality, there are lots of them.

Have you already tried that suggestion in the lkml?

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #30 from Dan Kegel   2009-11-27 15:29:00 ---
See also 
http://www.archivum.info/comp.lang.c/2005-07/01975/Re:_const-incorrect_pract...
for another example where somebody concluded he was a troll.

Markus, patches that solve real problems are welcome, but telling
us how to do our job isn't.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #31 from Markus Elfring   2009-11-27 16:06:08 ---
(In reply to comment #30)

Would you like to consider the attachment #15367 as an acceptable patch?

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #32 from Dan Kegel   2009-11-27 16:26:39 ---
Go ahead and submit it to wine-patches, see if Alexandre will accept it.
He's pretty picky, even extremely useful patches often get rejected for various
reasons.

I'd suggest making the string const local.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #33 from Dmitry Timoshkov   2009-11-27 22:34:26 ---
(In reply to comment #31)
> (In reply to comment #30)
> Would you like to consider the attachment #15367 [details] as an acceptable patch?

It's been explained to you in the comment #12 already, sig_atomic_t buys
you nothing either. For some reason you are stubbornly refusing to accept
that all your rant in this bug completely useless, and actually about
nothing. You are losing your time and ours, please try to find some other
activity.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #34 from Dmitry Timoshkov   2009-11-28 00:56:25 ---
> - fprintf( stderr, "wineserver crashed, please enable coredumps (ulimit -c unlimited) and restart.\n");
> - (void) write(STDERR_FILENO, crash_message, sizeof(crash_message) - 1);

And btw, you are breaking your own requirements stated in the bug 14788 you
are referencing here :-) Is there any reason why you don't follow your own
coding standard rules? That's a not safe programming practice, right? <grin>

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #35 from Markus Elfring   2009-11-28 07:58:33 ---
(In reply to comment #32)
> Go ahead and submit it to wine-patches, see if Alexandre will accept it.

http://www.winehq.org/pipermail/wine-patches/2009-November/081859.html
http://article.gmane.org/gmane.comp.emulators.wine.patches/75194
> He's pretty picky, even extremely useful patches often get rejected
> for various reasons.

Thanks for your background information. I am curious if dissension can be
resolved.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #36 from Markus Elfring   2009-11-28 08:02:47 ---
(In reply to comment #33)
> It's been explained to you in the comment #12 already, sig_atomic_t buys
> you nothing either.

The use of this data type improves the portability for setting of flags in
signal handlers.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #37 from Markus Elfring   2009-11-28 08:10:38 ---
(In reply to comment #34)
> And btw, you are breaking your own requirements stated in the bug 14788 you
> are referencing here :-) Is there any reason why you don't follow your own
> coding standard rules?

I do not see the need in this special case because I intentionally marked the
ignorance of the return value by the cast. Would you like to continue the
discussion of such software design details in the mentioned bug report?

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #38 from Vitaliy Margolen   2009-11-28 10:41:52 ---
Marcus, if you send patches, stick to the standards of _this_ project. DO NOT
invent your own standards. If you want to do that - go start your own.

You've been told do not add any bogus "(void)" anywhere, DO NOT use any
arbitrary types. If you can not comprehend this requirements, please stay away
from this project!

Dan, please remove his user from bugzilla, we don't need trolls here.

[Bug 14789] Fix signal handler

http://bugs.winehq.org/show_bug.cgi?id=14789

--- Comment #39 from Markus Elfring   2009-11-28 10:55:33 ---
(In reply to comment #38)
> DO NOT invent your own standards.

I have not got the impression that I do that. I am trying to reuse
specifications from POSIX and recent C as much as possible.
> You've been told do not add any bogus "(void)" anywhere, DO NOT use any
> arbitrary types. If you can not comprehend this requirements, please stay away
> from this project!

I am very surprised by your very strong reaction.
> Dan, please remove his user from bugzilla, we don't need trolls here.

I find it strange that I get classified as a troll because it seems that there
is still too many disagreement on technical details around so far.