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

[Esteban Lorenzano]

Hi, 

thanks!

> On 12 Jun 2015, at 04:26, David T. Lewis <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.

I follow it.
What I do not do is to have all plugins we use in separated packages: I think is a source of chaos :)
I update Pharo repository with versions of plugins regularly… then we do not have to rely in different repositories, etc. to have a working version. 

Esteban

> 
> 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>:
>>>> 
>>>>> 
>>>>> 
>>>>> 2015-06-11 15:35 GMT+02:00 Thierry Goubier <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>