Fix signal handler

[michaelrsweet]

Version: 2.1b1
CUPS.org User: elfring

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

• Variables like "abort_job" and "job_canceled" should use the data type "sig_atomic_t", shouldn't they?
• The functions "exit" and "fprintf" do not belong to the list of async-signal-safe functions.
  https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers

[michaelrsweet]

CUPS.org User: mike

Please be more specific - all of these bug reports are useless without context, and really I am not going to search and chase after vague references to potential issues.

WRT sig_atomic_t, it is just int on most platforms and we are using it to set a flag - there is no atomicy issue.

We shouldn't be calling exit or fprintf from signal handlers - please provide specific source file references.

[michaelrsweet]

CUPS.org User: elfring

Does your content management system provide a direct link interface for the affected source files?

Would your source code become a bit safer if a portable data type definition will be reused?
https://www.securecoding.cert.org/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers

[michaelrsweet]

CUPS.org User: mike

Markus, no we don't have a direct link interface - a plain old unified diff would be fine, a list of source files, etc.

As for using a "portable" data type definition, CUPS is still used on platforms that don't define the newer C11 data types - we'd need to add a configure check with a backup typedef to int. And then we'd likely want to do a shadow definition (cups_sigatomic_t) to avoid interactions with system headers that autoconf misses.

And after all of that we won't be any better off - we are just setting a flag when a signal is received so the code outside the signal handler is just looking for non-zero. This usage is safe on all platforms that CUPS supports, and certainly on all platforms I've ever worked on over the last 25 years of my professional career (and 8 years before that going back to 8-bit processors).

From the glibc manual on sig_atomic_t (http://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html):

In practice, you can assume that int is atomic. You can also assume
that pointer types are atomic; that is very convenient. Both of these
assumptions are true on all of the machines that the GNU C Library
supports and on all POSIX systems we know of.

In short, the amount of time we'd waste making this change (and the amount of time I'm spending responding to you) will not improve CUPS in the least but will increase code complexity and add to the already too large autoconf nonsense we have to deal with.

So please, if you have found instances where we are calling non-async safe functions from signal handlers, please tell me where those instances are (list of files or a diff) and I'll take a look. But let's avoid being pedantic for pedantistry's sake...

[michaelrsweet]

CUPS.org User: elfring

Do any special C/C++ compilers need an attribute like "volatile" in the data type for the mentioned variables?

Are the following function implementations update candidates?

sigterm_handler|backend/dnssd.c:1287
sigterm_handler|backend/usb-darwin.c:2246

[michaelrsweet]

CUPS.org User: elfring

Would you like to try a specific source code search out with a script like the following which reuses the semantic patch language?
http://coccinelle.lip6.fr/

@show_unsafe@
identifier receiver =~ "^(?x)
(?:
(?:alarm|parent|sig(?:chld|hup|term))_handler
|
handle_sigterm
|
(?:CancelJ|cancel_j)ob
)$";
@@
void receiver(...)
{
<+...
(

• exit
  |
• fprintf
  )
  (...);
  ...+>
  }

[michaelrsweet]

CUPS.org User: mike

Fixed in Subversion repository.

[michaelrsweet]

"str4671.patch":

Index: backend/dnssd.c

--- backend/dnssd.c (revision 12817)
+++ backend/dnssd.c (working copy)
@@ -1289,7 +1289,7 @@
(void)sig;

if (job_canceled)

• exit(CUPS_BACKEND_OK);
• _exit(CUPS_BACKEND_OK);
  else
  job_canceled = 1;
  }

Index: backend/usb-darwin.c

--- backend/usb-darwin.c (revision 12817)
+++ backend/usb-darwin.c (working copy)
@@ -2285,13 +2285,13 @@
while (waitpid(child_pid, &status, 0) < 0 && errno == EINTR);

if (WIFEXITED(status))

•  exit(WEXITSTATUS(status));
  

•  _exit(WEXITSTATUS(status));
  

  else if (status == SIGTERM || status == SIGKILL)

•  exit(0);
  

•  _exit(0);
  

  else
  {

•  fprintf(stderr, "DEBUG: Child crashed on signal %d\n", status);
  

•  exit(CUPS_BACKEND_STOP);
  

•  write(2, "DEBUG: Child crashed.\n", 22);
  

•  _exit(CUPS_BACKEND_STOP);
  

  }
  }
  #endif /* i386 || x86_64 */

[michaelrsweet]

CUPS.org User: elfring

Is the implementation of the function "sigterm_handler" from the source file "backend/ipp.c" another update candidate according to the commit ef416fc on 2006-01-13?

[elfring]

Will the mentioned implementation details get another look?

[michaelrsweet]

[master 9880f81] Don't call exit() from signal handler (Issue #4671)

[elfring]

Thanks for another small source code improvement.