search

trueos / trueos-core

59 stars 18 forks source link

PCDM-session does not wait for the right process ID #260

Closed jdebp closed 7 years ago

jdebp commented 7 years ago

Consider the code in main():

  int sid = -1;
  int pid = fork();
  if(pid < 0){
    ...
  }else if( pid ==0 ){
    sid = setsid(); //start a session
    ...
  }else{ 
    ...
    waitpid(sid,&status,0); //wait for the child (session) to finish

The sid variable is always -1 in the parent process, and waitpid() ends up waiting for the wrong thing, going very wrong if some other child process happens to terminate. waitpid() should be waiting for the process ID given in pid:

    waitpid(pid,&status,0); //wait for the child process to finish

Of course, this whole forking mechanism is entirely superfluous, which is another issue. But fixing this at least makes PCDM-session robust against multiple children, which it is not now. As it stands, if some other child process terminates, which can happen if local reaping becomes involved, PCDM-session drops the entire login session prematurely, because this bug has made it wait for process ID -1 rather than for the ID of the child process that it forked.

pkgdemon commented 7 years ago

If you have tested this change, and can confirm it doesn't break anything please submit a PR for this change to be reviewed.

https://github.com/trueos/pcdm/tree/master/src-qt5

jdebp commented 7 years ago

That is wrong, not least because this is exhibited in a pre-OpenRC version of the system. Clearly, this code is not a hack to make virtual terminal allocation work. It doesn't backgroundize anything. Nor does it deal in virtual terminals. It makes a parent process wait for the wrong child process ID.

The hacks to make virtual terminals work are the sleeps and the explicit virtual terminal name in PCDMd. We're looking at PCDM-session here, a completely different program. Please look at the right program and read the bug report again, because the the idea that this should be closed without fixing the program to wait for the right process ID seems very wrong.

pkgdemon commented 7 years ago

Yes I copied my generic response from the other ticket. One thing was common so I have edited my response to reflect that. Please send a pull request so your code can be reviewed, and not a bug report. Thanks.

jdebp commented 7 years ago

It was wrong in #261, too.

pkgdemon commented 7 years ago

Updated the message in #261 to simply request a PR.

beanpole135 commented 7 years ago

I just wanted to point out that the code block you mention is there, but I am fairly certain I left disabled/disused via a compile flag in the main() function. That was just some random code I left in there while testing out an alternative to the PCDMd startup script. I may resurrect it later on with version 2 of PCDM, but my available time is still too limited to tackle this right now.

jdebp commented 7 years ago

No, there is no compile flag. The aforegiven code is most definitely executed, and two PCDM-session processes appear in the process list because of it. That's wholly unnecessary, as explained in #261. But failing getting rid if it in its entirety, you could at least fix the one character error that is causing it to wait for the wrong process ID and break. Hence this simpler bug report to start you off. (-: