Page MenuHomePhabricator

Use "exec" to skip the Dash intermediate shell for "pygmentize"
ClosedPublic

Authored by epriestley on Jan 29 2019, 2:04 PM.
Tags
None
Referenced Files
F14309111: D20054.diff
Wed, Dec 18, 12:45 AM
Unknown Object (File)
Wed, Dec 11, 1:16 PM
Unknown Object (File)
Sun, Dec 8, 8:49 PM
Unknown Object (File)
Thu, Dec 5, 8:33 AM
Unknown Object (File)
Wed, Dec 4, 7:21 PM
Unknown Object (File)
Wed, Nov 27, 2:45 AM
Unknown Object (File)
Mon, Nov 25, 8:00 AM
Unknown Object (File)
Sat, Nov 23, 2:25 PM
Subscribers
None

Details

Summary

See T13224. A user reports the setTimeout() not actually killing Pygments under Ubuntu, where the default shell is "dash".

I think what's happening here is:

  • Dash creates a strict subprocess always, Bash only does sometimes. If you "bash" a single simple command, it automatically behaves like you "exec"'d it and replaces itself.
  • So we end up with "dash" as our subprocess, and then "pygments" under it.
  • setTimeout() does SIGTERM + wait + SIGKILL.
  • When we SIGTERM, I'd guess it propagates to "pygments" but "pygments" ignores it (it's stuck inside regexp explosion land).
  • When we SIGKILL, I think it kills "dash" and leaves "pygments" alive.

I didn't really test any of this since it's theoretically sound and adding "exec" is generally safe, and the reporting user says it fixed things.

Test Plan

Highlighted some non-problematic code locally to make sure this didn't break anything in the working case.

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/markup/syntax/highlighter/PhutilPygmentsSyntaxHighlighter.php
25

I honestly thought "dash" was a typo in T13224.

This revision is now accepted and ready to land.Jan 30 2019, 6:25 AM
This revision was automatically updated to reflect the committed changes.