OpenLDAP
Up to top level
Build   Contrib   Development   Documentation   Historical   Incoming   Software Bugs   Software Enhancements   Web  

Logged in as guest

Viewing Development/5248
Full headers

From: h.b.furuseth@usit.uio.no
Subject: non-atomic signal variables
Compose comment
Download message
State:
0 replies:
8 followups: 1 2 3 4 5 6 7 8

Major security issue: yes  no

Notes:

Notification:


Date: Tue, 27 Nov 2007 22:24:29 GMT
From: h.b.furuseth@usit.uio.no
To: openldap-its@OpenLDAP.org
Subject: non-atomic signal variables
Full_Name: Hallvard B Furuseth
Version: HEAD
OS: 
URL: 
Submission from: (NULL) (129.240.6.233)
Submitted by: hallvard


Some non-atomic variables are used both outside and inside signal handlers:

clients/tools/common.c:
	static int	gotintr;
	static int	abcan;
servers/slapd/daemon.c:
	static volatile int waking;
	(used via WAKE_LISTENER in signal handler)
slapd/slapcat.c:
	static int gotsig;

They need to be volatile sig_atomic_t.
That can be signed or unsigned char, so abcan must be set to some
#define in range 0..127 instead of -1.

Two more problems:

WAKE_LISTENER will not be correct anyway since it does '++waking' (not
atomic) and is called both inside and outside a signal handler.
The !NO_THREADS version is OK.

For that matter, behavior is only defined when the signal handler
_writes_ a volatile sig_atomic_t, not when it _reads_ it:
  http://groups.google.no/group/comp.std.c/msg/d01196111ce93615
Dunno what the problem is, or if it is worse than variables
accessed by threads.  I don't see anything to do about it either.


Followup 1

Download message
Date: Wed, 28 Nov 2007 14:54:45 -0800
From: Howard Chu <hyc@symas.com>
To: h.b.furuseth@usit.uio.no
CC: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
h.b.furuseth@usit.uio.no wrote:
> Full_Name: Hallvard B Furuseth
> Version: HEAD
> OS:
> URL:
> Submission from: (NULL) (129.240.6.233)
> Submitted by: hallvard
> 
> 
> Some non-atomic variables are used both outside and inside signal handlers:
> 
> clients/tools/common.c:
> 	static int	gotintr;
> 	static int	abcan;
> servers/slapd/daemon.c:
> 	static volatile int waking;
> 	(used via WAKE_LISTENER in signal handler)
> slapd/slapcat.c:
> 	static int gotsig;
> 
> They need to be volatile sig_atomic_t.

> That can be signed or unsigned char, so abcan must be set to some
> #define in range 0..127 instead of -1.
> 
> Two more problems:
> 
> WAKE_LISTENER will not be correct anyway since it does '++waking' (not
> atomic) and is called both inside and outside a signal handler.
> The !NO_THREADS version is OK.
> 
> For that matter, behavior is only defined when the signal handler
> _writes_ a volatile sig_atomic_t, not when it _reads_ it:
>   http://groups.google.no/group/comp.std.c/msg/d01196111ce93615
> Dunno what the problem is, or if it is worse than variables
> accessed by threads.  I don't see anything to do about it either.

No. volatile is used in daemon.c, yes. It is not needed in clients/tools or 
slapcat.

sig_atomic_t is irrelevant in slapcat. Whether we detect zero/non-zero 
immediately, one entry, or two entries after the signal occurs isn't going to 
make any difference.

Likewise, there's no issue with gotintr/abcan. The signal handler isn't armed 
until the writes are complete. Therefore whether it can be read atomically or 
not inside the handler is irrelevant, the value is constant.

And of course...
http://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html#Atomic-Types

The C standard defines "int" to be the most efficient machine word, and that 
is always atomic.

-- 
   -- Howard Chu
   Chief Architect, Symas Corp.  http://www.symas.com
   Director, Highland Sun        http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP     http://www.openldap.org/project/



Followup 2

Download message
From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
Date: Thu, 29 Nov 2007 01:03:38 +0100
To: Howard Chu <hyc@symas.com>
Cc: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
[rearranging]
Howard Chu writes:

> The C standard defines "int" to be the most efficient machine word,
> and that is always atomic.

It does neither, that I can see.  Among other things because "most
efficient" is not well-defined.  E.g. by space or time?  Efficient with
which operations?  It's the recommended intent somewhere (the
Rationale?), but that must be subject to whatever other restrictions an
implementor faces.  (16+ bits, backwards compatibility, etc.)

And the compiler can choose between an atomic and non-atomic operation,
even with volatile.  Volatile guarantees a change is complete at a
sequence point.  Signals need not arrive at sequence points.  (C99
5.1.2.3p2,4.  You can download the standard with amendments free at
<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf>.)

Otherwise the standard would not have needed to define sig_atomic_t.

> sig_atomic_t is irrelevant in slapcat. Whether we detect zero/non-zero
> immediately, one entry, or two entries after the signal occurs isn't
> going to make any difference.

Well, I don't see any reason not to use it when the standard says so and
the change is trivial, even if we don't know of a real-life failure.
(E.g. if the compiler detects that the variable will not legally change
and optimizes it away, or if it is being read during a signal which
changes it so the result of the non-atomic read is a trap
representation.)

> Likewise, there's no issue with gotintr/abcan. The signal handler
> isn't armed until the writes are complete. Therefore whether it can be
> read atomically or not inside the handler is irrelevant, the value is
> constant.

So reading abcan is safe, but reading gotintr is not.  Read first half
before signal as 0, last half as -1.  Then the switch on it fails.

-- 
Hallvard



Followup 3

Download message
Date: Wed, 28 Nov 2007 22:42:58 -0800
From: Howard Chu <hyc@symas.com>
To: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
CC: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
Hallvard B Furuseth wrote:
> [rearranging]
> Howard Chu writes:
> 
>> The C standard defines "int" to be the most efficient machine word,
>> and that is always atomic.
> 
> It does neither, that I can see.  Among other things because "most
> efficient" is not well-defined.  E.g. by space or time?  Efficient with
> which operations?  It's the recommended intent somewhere (the
> Rationale?), but that must be subject to whatever other restrictions an
> implementor faces.  (16+ bits, backwards compatibility, etc.)

Backward compatibility only reinforces my point, since int has always been 
atomic in the past. Likewise when migrating old code to new hardware, machine 
word sizes only grow if they change at all, so any word small enough to be 
atomic on old hardware is also atomic on new.

> And the compiler can choose between an atomic and non-atomic operation,
> even with volatile.  Volatile guarantees a change is complete at a
> sequence point.  Signals need not arrive at sequence points.  (C99
> 5.1.2.3p2,4.  You can download the standard with amendments free at
> <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf>.)

Hm, seems I have a much older draft, this language in 6.2.5 Types has changed.

> Otherwise the standard would not have needed to define sig_atomic_t.

Note that sig_atomic_t is an *optional* part of the standard; implementations 
are not required to provide it.

>> sig_atomic_t is irrelevant in slapcat. Whether we detect zero/non-zero
>> immediately, one entry, or two entries after the signal occurs isn't
>> going to make any difference.
> 
> Well, I don't see any reason not to use it when the standard says so and
> the change is trivial, even if we don't know of a real-life failure.
> (E.g. if the compiler detects that the variable will not legally change
> and optimizes it away, or if it is being read during a signal which
> changes it so the result of the non-atomic read is a trap
> representation.)

The standard says it is impossible for arithmetic operations on valid values 
to generate trap representations. Think about it - if it were *possible* for 
intermediate states to be invalid, then it would be *inevitable* that they 
occur in normal code and you'd be producing traps practically all the time, at 
which point the only way to get useful work out of the hardware would be to 
disable the trap mechanism completely (e.g. mask off parity error exception 
generation). Nobody is stupid enough to design a machine like that, and the 
standard guarantees that programmers never have to worry about such a 
stupidity ever existing.

>> Likewise, there's no issue with gotintr/abcan. The signal handler
>> isn't armed until the writes are complete. Therefore whether it can be
>> read atomically or not inside the handler is irrelevant, the value is
>> constant.

> So reading abcan is safe, but reading gotintr is not.  Read first half
> before signal as 0, last half as -1.  Then the switch on it fails.

Not on any machine ever built (for which a C translator exists).

OK, you're right, it's trivial, and I see that the configure script already 
#defines sig_atomic_t to int if the system doesn't already define it. So your 
suggested changes are harmless, go ahead and make them. If you're going to 
that trouble, you should change gotintr to use an explicitly defined 
contiguous range of values.
-- 
   -- Howard Chu
   Chief Architect, Symas Corp.  http://www.symas.com
   Director, Highland Sun        http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP     http://www.openldap.org/project/



Followup 4

Download message
From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
Date: Thu, 29 Nov 2007 16:17:47 +0100
To: Howard Chu <hyc@symas.com>
Cc: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
Howard Chu writes:
> OK, you're right, it's trivial, and I see that the configure script
> already #defines sig_atomic_t to int if the system doesn't already
> define it. So your suggested changes are harmless, go ahead and make
> them. If you're going to that trouble, you should change gotintr to
> use an explicitly defined contiguous range of values.

OK, later.. that's why I filed an its first (in particular, need to look
at WAKE_LISTENER first.)


Which makes the rest of this reply irrelevant to the ITS, so skip it if
that helps your blood pressure:

> Backward compatibility only reinforces my point, since int has always
> been atomic in the past.

Where do you get that from?  A brief search find nothing like that in
the draft and standard versions I've lying around.  It would also mean
that sig_atomic_t would be a pointless type.

>> Otherwise the standard would not have needed to define sig_atomic_t.
>
> Note that sig_atomic_t is an *optional* part of the standard;
> implementations are not required to provide it.

Irrelevant.  Freestanding implementations need not provide it (nor
most of the rest of the C library), but <signal.h> requires it.

> The standard says it is impossible for arithmetic operations on valid
> values to generate trap representations. Think about it - if it were
> *possible* for intermediate states to be invalid, (...)

A signal breaks the rest of the execution model.  A variable need not be
in an "intermediate state" according to that model when a signal
arrives.  Otherwise C could not use *any* inherently non-atomic types,
which would rather limit which CPUs could support C.  That's why the
standard says signal handlers can do so aburdly few things in the first
place.

> Nobody is stupid enough to design a machine like that, and the
> standard guarantees that programmers never have to worry about such a
> stupidity ever existing.
> (...)
> Not on any machine ever built (for which a C translator exists).

On the countrary, the existence of such hosts is precicely why the
section about signal handlers explicitly contradicts you.  And as Doug
Gwyn said, even the little part it does guarantee was hard to get
agreement for.

-- 
Regards,
Hallvard



Followup 5

Download message
From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
Date: Thu, 6 Dec 2007 17:48:57 +0100
To: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
Fixed the easy cases in HEAD.
WAKE_LISTENER under --without-threads remains.

-- 
Hallvard



Followup 6

Download message
Date: Thu, 06 Dec 2007 09:11:10 -0800
From: Howard Chu <hyc@symas.com>
To: h.b.furuseth@usit.uio.no
CC: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
h.b.furuseth@usit.uio.no wrote:
> Fixed the easy cases in HEAD.
> WAKE_LISTENER under --without-threads remains.
> 
As I recall, the only reason we needed the waking counter here was to make 
sure we never filled the pipe buffer, which would cause a single-threaded 
server to deadlock. We can simply set the pipe to nonblocking instead, and 
eliminate the counter.

The other point was that if there was already a pending wake event, there was 
no reason to write another one.
-- 
   -- Howard Chu
   Chief Architect, Symas Corp.  http://www.symas.com
   Director, Highland Sun        http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP     http://www.openldap.org/project/



Followup 7

Download message
From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
Date: Thu, 6 Dec 2007 19:33:59 +0100
To: hyc@symas.com
Cc: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
hyc@symas.com writes:
> As I recall, the only reason we needed the waking counter here was to
> make sure we never filled the pipe buffer, which would cause a
> single-threaded server to deadlock. We can simply set the pipe to
> nonblocking instead, and eliminate the counter.

Sounds good...

> The other point was that if there was already a pending wake event,
> there was no reason to write another one.

Aha!  Now the code makes more sense to me:-)  And if 'waking' is
just a boolean flag, we can keep it as long as it is treated that way.

#define WAKE_LISTENER(w) do { \
	if ((w) && !waking) { \
		waking = 1; \
		tcp_write( SLAP_FD2SOCK(wake_sds[1]), "0", 1 ); \
	} \
} while (0)

I need to stare at it a bit to look for race conditions with readers.
I don't suggest to hold next RE24 for this though.  It's hardly urgent,
and should be tested a bit --without-threads first.

-- 
Regards,
Hallvard



Followup 8

Download message
Date: Thu, 08 May 2008 09:44:37 -0700
From: Howard Chu <hyc@symas.com>
To: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
CC: openldap-its@openldap.org
Subject: Re: (ITS#5248) non-atomic signal variables
Hallvard B Furuseth wrote:
> hyc@symas.com writes:
>> As I recall, the only reason we needed the waking counter here was to
>> make sure we never filled the pipe buffer, which would cause a
>> single-threaded server to deadlock. We can simply set the pipe to
>> nonblocking instead, and eliminate the counter.
>
> Sounds good...
>
>> The other point was that if there was already a pending wake event,
>> there was no reason to write another one.
>
> Aha!  Now the code makes more sense to me:-)  And if 'waking' is
> just a boolean flag, we can keep it as long as it is treated that way.
>
> #define WAKE_LISTENER(w) do { \
> 	if ((w)&&  !waking) { \
> 		waking = 1; \
> 		tcp_write( SLAP_FD2SOCK(wake_sds[1]), "0", 1 ); \
> 	} \
> } while (0)
>
> I need to stare at it a bit to look for race conditions with readers.
> I don't suggest to hold next RE24 for this though.  It's hardly urgent,
> and should be tested a bit --without-threads first.
>
Actually this can all be simplified even further if you're building 
--without-threads - in that case, the only WAKE_LISTENER call that matters is 
the one made from the signal handler. In every other case, the calling 
function will eventually return and fall back into the main event loop anyway.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/


Up to top level
Build   Contrib   Development   Documentation   Historical   Incoming   Software Bugs   Software Enhancements   Web  

Logged in as guest


The OpenLDAP Issue Tracking System uses a hacked version of JitterBug

______________
© Copyright 2013, OpenLDAP Foundation, info@OpenLDAP.org