Re: Broken su in current - trying to fix myself, help needed!

韓家標 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