search

rakitzis / rc

rc shell -- independent re-implementation for Unix of the Plan 9 shell (from circa 1992)
Other
250 stars 23 forks source link

Drop all control flow entries from estack after fork #86

Closed xyb3rt closed 1 year ago

xyb3rt commented 1 year ago

Fixes issue #68.

xyb3rt commented 1 year ago

Is this ok with you, @rakitzis?

rakitzis commented 1 year ago

This looks good. How about an update to trip.rc to cover the change?

xyb3rt commented 1 year ago

I've moved the call to clearflow() directly into rc_fork(). Shouldn't this be done to all the calls to setsigdefaults(FALSE) too, @rakitzis? There's only one call to rc_fork() in backq() where the child does not call it and I'm not sure if this omission was done intentionally.

rakitzis commented 1 year ago

This may indeed be an oversight. It may be easy to test the backq use case since setsigdefaults clears sigexit, so there is a deterministic way to provoke bad behavior.

rakitzis commented 1 year ago

So it's really not obvious, but I traced this down.. may be worth documenting in backq after calling fork(): since the backquoted command is invoked by exec(), and since exec() calls setsigdefaults, restoring signals is not needed in the setup after fork().

rakitzis commented 1 year ago

It also seems like setsigdefaults is called redundantly in many cases. In fact, perhaps in all the cases excluding backq!

The logic here is quite twisted. I can't say I'm proud of it 30 years on. I would leave it alone, as a real fix may involve keeping track of some sort of state which needs to be applied after fork() and before exec(). Doing this via a collection of booleans sprinkled through the code is very brittle.