From owner-freebsd-current@FreeBSD.ORG Thu Oct 18 13:34:36 2007 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BC14616A421 for ; Thu, 18 Oct 2007 13:34:36 +0000 (UTC) (envelope-from matrix@itlegion.ru) Received: from corpmail.itlegion.ru (corpmail.itlegion.ru [84.21.226.211]) by mx1.freebsd.org (Postfix) with SMTP id 0AD0913C48D for ; Thu, 18 Oct 2007 13:34:35 +0000 (UTC) (envelope-from matrix@itlegion.ru) Received: (qmail 65774 invoked from network); 18 Oct 2007 17:34:33 +0400 Received: from unknown (HELO Artem) (192.168.0.12) by 84.21.226.211 with SMTP; 18 Oct 2007 17:34:33 +0400 X-AntiVirus: Checked by Dr.Web [version: 4.44, engine: 4.44.0.09170, virus records: 250035, updated: 18.10.2007] Message-ID: <01b801c8118b$9baa1340$0c00a8c0@Artem> From: "Artem Kuchin" To: =?UTF-8?B?6Z+T5a625qiZIEJpbGwgSGFja2Vy?= , References: <00bd01c810ec$10371230$0c00a8c0@Artem> <8cb6106e0710171143m3dff7546o457192ede76e6598@mail.gmail.com> <012c01c810f3$aafeecf0$0c00a8c0@Artem> <20071017193615.GO9006@server.vk2pj.dyndns.org> <471667DB.1010601@conducive.net> <47170FF1.3050602@moneybookers.com> <471746C7.20306@conducive.net><47174BE4.6020300@moneybookers.com> <4717523E.1000403@conducive.net><010f01c81184$cd375550$0c00a8c0@Artem> <47175E94.6090309@conducive.net> Date: Thu, 18 Oct 2007 17:34:27 +0400 Organization: IT Legion MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset="UTF-8"; reply-type=response Content-Transfer-Encoding: 8bit X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 6.00.2900.3138 X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.3198 Cc: Subject: Re: Broken su in current - trying to fix myself, help needed! X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Oct 2007 13:34:36 -0000 韓家標 Bill Hacker wrote: > Artem Kuchin wrote: >>> What Artem is seeing is not (yet) a 'bug' in su in my mind. >>> >> >> You missed reply from David Xu in the list on this matter. >> > > No, I saw it.. I just have a different 'take' on it. Two actually.. > s.b. > >> To me there is CLEARLY a bug in the source code. It tried >> to get group of already dead process. >> >> Here is quote from my and David's letters: >> >>> The weird thing is that if i just comment out those lines like this >>> >>> /* child_pgrp = getpgid(child_pid); >>> if (tcgetpgrp(STDERR_FILENO) == child_pgrp) */ >>> tcsetpgrp(STDERR_FILENO, getpgrp()); >>> >>> su starts working again just fine. >>> >>> Any idea why getpgid fails and why tcgetpgrp return 100000 (always >>> the same >>> number)? What will brak if i leave these lines commented? >>> >>> -- >>> Regards, >>> Artem >> >> file su.c, line 472 may be incorrect since line 456 is a while loop >> which only >> exits if child process is exited. just remove line 472 and 473 to >> see if problem >> is fixed. >> > Specifically - *why* was it coded that way to begin with? I'm sure > 'su' has had lots of peer review and rewrite since V1 ATT UNIX. Just MAUBE something was really badly broken in waitpid or lower, so it was coded this way. But as i see it is no longer broken there and su is broken here. > Perhaps I'm overly conservative, but one has to ask - should there be > more selective code *added* to handle the case of a missing child > process pid and carry on, rather than removing that snippet of code? Well, if you look at cvs history this strange code was added not so long ago to fix something which came up back then. I think it is no longer needed. > JMNSHO, but 'su' is too important, in too many places to be trifled > with lightly. Yes, and it su is broken now and must be fixed because it is important. > So - your query 'What will break if..' is a good starting point. More review and testing is in order. It is quite possible then commiter to su just overlooked this code error because they do no use su the way some people do (like me) and they have never seen this bug. I have tested the cases uneder all shells and su does not work correctly anywhere, so it is SU's fault. So, i ask people to fix their su and play around a bit and then submit patch to cvs. I am not a submitter and have no clue about the procedure. -- Artem