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

[Esteban Lorenzano]

another btw… 

right now implementation of primitive is very different in oscog branch and interpreter branch. 
for now I just took the change of vfork for fork, but… shouldn’t those branches be more alike? 


Esteban

> On 12 Jun 2015, at 11:55, Esteban Lorenzano <estebanlm at gmail.com> wrote:
> 
> 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> 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>:
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 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>
>