X-Recipient: archive-cygwin AT delorie DOT com
X-Spam-Check-By: sourceware.org
Date: Fri, 23 Nov 2012 15:18:30 +0100
From: Corinna Vinschen <corinna-cygwin AT cygwin DOT com>
To: cygwin AT cygwin DOT com
Subject: Re: SYSV semaphore bug (testcase attached)
Message-ID: <20121123141830.GB24913@calimero.vinschen.de>
Reply-To: cygwin AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
References: <5F8AAC04F9616747BC4CC0E803D5907D012656 AT MLBXV09 DOT nih DOT gov> <20121123121942 DOT GP17347 AT calimero DOT vinschen DOT de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20121123121942.GP17347@calimero.vinschen.de>
User-Agent: Mutt/1.5.21 (2010-09-15)
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Precedence: bulk
List-Id: <cygwin.cygwin.com>
List-Unsubscribe: <mailto:cygwin-unsubscribe-archive-cygwin=delorie DOT com AT cygwin DOT com>
List-Subscribe: <mailto:cygwin-subscribe AT cygwin DOT com>
List-Archive: <http://sourceware.org/ml/cygwin/>
List-Post: <mailto:cygwin AT cygwin DOT com>
List-Help: <mailto:cygwin-help AT cygwin DOT com>, <http://sourceware.org/ml/#faqs>
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
Delivered-To: mailing list cygwin AT cygwin DOT com

On Nov 23 13:19, Corinna Vinschen wrote:
> On Nov 19 16:04, Lavrentiev, Anton (NIH/NLM/NCBI) [C] wrote:
> > Hi,
> > 
> > As I previously reported, there is a weird behavior of CYGWIN implementation
> > of SYSV semaphores, and a bug exposition for one problem is attached below
> > (I'm still looking into some other issues in there).
> > 
> > If the code is compiled with both BUG1 and BUG2 defined (as shown), it
> > will abort at iteration 16384 (just the default semaphore overflow).
> > 
> > Undefining BUG2 causes the problem disappear (because there is no longer
> > UNDO on the 1st semaphore).  Also, running the code with just one semaphore
> > (undef BUG1) causes no problem.  Finally, replacing "#if 1" with "#if 0"
> > to unlock the semaphore allows to run indefinitely with any combination of
> > BUG1/2 (just remember to issue ipcs/ipcrm to start with a clean slate at
> > all times).
> > 
> > Reviewing the code of CYGSERVER, there is an apparent bug in the semundo_clear()
> > routine (at around line 536, which looks like "i++, sunptr++;", and advances
> > both undo indexes even when "if (sunptr->un_id == semid)" (line 524)
> > failed to match semid.  This means that for two (or more) semaphores, the
> > undo index "i" moves ahead even when nothing was done while still searching.
> > This causes the adjust pointer to miss the position to clear, and overflow
> > the semaphore adjust value (line 1207, semop(), by the virtue of
> > semundo_adjust()'s logic at about line 486).
> 
> This is original FreeBSD code, so I have a hard time to follow the idea
> that it might be wrong.  I stared a long while into the source now, and
> I compared that with the latest version of the upstream code.
> 
> The i counter is in lockstep with the sunptr pointer.  The only time
> something happens is if sunptr->un_num (== suptr->un_ent[i].un_num)
> equals semnum.  In that case, suptr->un_ent[i] is overwritten with
> with the last element uptr->un_ent[suptr->un_cnt], and then the code
> calls continue, thus NOT incrementing i and sunptr.  So the same
> element, now containing the contents of suptr->un_ent[suptr->un_cnt],
> is evaluated again.
> 
> Am I missing something?

Yes, I do, and debugging as well as comparing my observations with the
NetBSD code of this function reveals a long-standing bug in the FreeBSD
code.  This call:

	if (semnum != -1)
		break;

is outside of the `if (semnum == -1 || sunptr->un_num == semnum)'
condition, and that results in ignoring every member of the un_ent array
but the first one (i == 0).

Can you please test the below patch?


Thanks,
Corinna


	* sysv_sem.cc (semundo_clear): Move condition to break from
	inner loop to the right spot.


Index: sysv_sem.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygserver/sysv_sem.cc,v
retrieving revision 1.10
diff -u -p -r1.10 sysv_sem.cc
--- sysv_sem.cc	6 Feb 2008 22:30:38 -0000	1.10
+++ sysv_sem.cc	23 Nov 2012 14:18:21 -0000
@@ -529,9 +529,9 @@ semundo_clear(int semid, int semnum, str
 						  suptr->un_ent[suptr->un_cnt];
 						continue;
 					}
+					if (semnum != -1)
+						break;
 				}
-				if (semnum != -1)
-					break;
 			}
 			i++, sunptr++;
 		}


-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple