[Vm-dev] Critical OSPP fix (was: OSProcess and lost SIGCHLD)

Esteban Lorenzano estebanlm at gmail.com
Fri Jun 12 11:15:48 UTC 2015


of course :)

> On 12 Jun 2015, at 13:10, Thierry Goubier <thierry.goubier at gmail.com> wrote:
> 
> Hi Esteban,
> 
> Can I do a pull request on that one?
> 
> There are a few things I'd like to see changed apart from the fork to vfork change, to improve some of the error handling. As the code is now, it is impossible, from Pharo, to make the difference between a git commit with no data to commit and a fail in the chdir.
> 
> Thierry
> 
> 2015-06-12 11:55 GMT+02:00 Esteban Lorenzano <estebanlm at gmail.com <mailto:estebanlm at gmail.com>>:
> btw… I cherry picked the change for oscog branch, but since I do not have rights I cannot push it back (another argument agains multiple separated repositories)
> 
> Esteban
> 
> > On 12 Jun 2015, at 04:26, David T. Lewis <lewis at mail.msen.com <mailto:lewis at mail.msen.com>> wrote:
> >
> > Eliot, Esteban:
> >
> > Thiery Goubier has identified and fixed a critical bug in OSProcessPlugin. The
> > issue was improper use of vfork() in a situation where fork() should be used. I
> > apparently made this mistake many years ago, and I can't think of any good excuse
> > for it. But I am very happy that Thiery has figured it out and fixed it after all
> > this time :-)
> >
> > The fix is in VMConstruction-Plugins-OSProcessPlugin-dtl.40 and updated trunk
> > SVN sources.
> >
> > It is important that we get this fix into the oscog and Pharo branches of OSPP,
> > particularly because Thierry is running a Pharo VM and he needs the updates for
> > work that he is doing there.
> >
> > Thierry provided the fix in the attached MCZ, which is relative to
> > VMConstruction-Plugins-OSProcessPlugin.oscog-EstebanLorenzano.43 from the Pharo
> > repository. This is derived from the oscog branch of OSPP, maybe a bit out of
> > date but that is not important WRT this change.
> >
> > @Eliot: My suggestion would be to update to methods in UnixOSProcessPlugin method
> > category 'primitives - fork and exec' to match the latest in the main branch. Alternatively
> > you can copy the logic from #primitiveForkExec into the ##forkAndExecInDirectory: in
> > your oscog branch (that method no longer exists in my main branch). The relevant
> > snippet is:
> >
> >       pid := self fork. "cCode: 'fork()' "
> >       pid < 0
> >               ifTrue: [ self perror: 'fork'.
> >                       ^self primitiveFail ].
> >       pid = 0
> >               ifFalse:
> >                       [ "Normal return to Smalltalk - this is the old parent process."
> >                       "Enable the timer again before resuming Smalltalk."
> >                       self cCode: 'setitimer (ITIMER_REAL, &saveIntervalTimer, 0L)'.
> >                       interpreterProxy pop: 10; pushInteger: pid
> >                       "Pop 9 arguments plus receiver, push pid."]
> >               ifTrue:
> >                       [ "This is the new child process"
> >                       ...
> >
> > @Esteban: You will probably want to follow Eliot's branch, but you can provide
> > an immediate fix for Pharo by using Thierry's MCZ attached.
> >
> > Dave
> >
> >
> > On Thu, Jun 11, 2015 at 09:08:16PM +0200, Thierry Goubier wrote:
> >> Le 11/06/2015 20:07, David T. Lewis a ?crit :
> >>> Thank you!
> >>>
> >>> I am traveling, will follow up as soon as I am able.
> >>
> >> No worries, it will take a while to percolate to the final users.
> >>
> >> I'm testing it and I now have a nice result: if you enter a non-existent
> >> target directory for the fork, you will get the chdir error message on
> >> the stderr pipe inside Pharo :)
> >>
> >> Thierry
> >>
> >>> Dave
> >>>
> >>>> Ok, here is a VMConstruction-Plugins-OSProcessPlugin.oscog mcz... Only
> >>>> changes are in UnixOSProcessPlugin>>#forkAndExecInDirectory: . I'm using
> >>>> Esteban's version of the plugin as ancestor.
> >>>>
> >>>> I have an account now on squeaksource, at TppG  :)
> >>>>
> >>>> Thierry
> >>>>
> >>>> 2015-06-11 16:32 GMT+02:00 Thierry Goubier <thierry.goubier at gmail.com <mailto:thierry.goubier at gmail.com>>:
> >>>>
> >>>>>
> >>>>>
> >>>>> 2015-06-11 15:35 GMT+02:00 Thierry Goubier <thierry.goubier at gmail.com <mailto:thierry.goubier at gmail.com>>:
> >>>>>
> >>>>>> I'm in the slang, and I have a question:
> >>>>>>
> >>>>>> Does it makes sense to call a primitiveFail in the child process after
> >>>>>> the fork? The parent will never get it, isn't it, and the child will
> >>>>>> exit
> >>>>>> immediately (and only return its exit status).
> >>>>>>
> >>>>>> (I suspect I didn't get the message 'non existent directory' because
> >>>>>> the
> >>>>>> primitiveFailFor: reset errno and hence the perror("chdir") prints
> >>>>>> nothing)
> >>>>>>
> >>>>> Wrong, I got the perror, but couldn't make sense of it.
> >>>>>
> >>>>> OSProcess should return an error if a fork returns a pid of -1, and
> >>>>> never
> >>>>> let the process run or read data from the pipes.
> >>>>>
> >>>>> The question about primitiveFail in the child still holds :)
> >>>>>
> >>>>> I made the change in the UnixOSProcessPlugin in slang, on the pharo-vm
> >>>>> distrib; however, there are differences with your version
> >>>>> (VMConstruction-Plugins-OSProcessPlugin.oscog-eem.42), I'll have a look
> >>>>> at
> >>>>> backporting them.
> >>>>>
> >>>>> Thierry
> >>>>>
> >>>>
> >>>
> >>>
> > <VMConstruction-Plugins-OSProcessPlugin.oscog-ThierryGoubier.44.mcz>
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20150612/ddae5e15/attachment-0001.htm


More information about the Vm-dev mailing list