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

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.
> 
> -- 
> Artem
> 

Agree that *seems to fix* the immediate issue.  But - it may be treating the 
symptom, not the underlying problem.

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.

T'would be 'of interest', given how much re-write or auditing has been done to 
two such, to see how OpenBSD and DragonFlyBSD have altered it. Or if they have.

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?

"Working again just fine' has yet to be proven to not break something else under 
some other set of circumstances. Or open an exploitable hole.

JMNSHO, but 'su' is too important, in too many places to be trifled with lightly.

So - your query 'What will break if..' is a good starting point.  More review 
and testing is in order.

Best,

Bill