Skip to content

fix(lua): replace os.spawn with io.popen to prevent fd-inheritance corruption - #1759

Open
lbssousa wants to merge 1 commit into
gregorio-project:developfrom
lbssousa:fix-issue-1757
Open

fix(lua): replace os.spawn with io.popen to prevent fd-inheritance corruption#1759
lbssousa wants to merge 1 commit into
gregorio-project:developfrom
lbssousa:fix-issue-1757

Conversation

@lbssousa

@lbssousa lbssousa commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Replaces os.spawn() with io.popen() at both call sites in tex/gregoriotex.lua where a gregorio child process is launched.
  • Adds three helpers (gre_shellquote, gre_build_cmdstr, gre_use_popen) defined once, before get_prog_output.
  • Windows is unaffected: gre_use_popen is false when os.type == 'windows', preserving the original os.spawn() path.

Problem

os.spawn() on Unix is implemented as fork()+execve() without setting FD_CLOEXEC on any file descriptor. The gregorio child process therefore inherits every fd open in the LuaTeX parent — including the fd of any .tex file currently being \input-ted. Parent and child share the same kernel-level struct file (which holds the seek position f_pos); any fd activity in the child can silently advance the read offset LuaTeX still relies on, corrupting TeX's input state.

Both call sites are affected:

Call site Reached via
get_prog_output() \gabcsnippet → direct_gabc()
compile_gabc() \gregorioscore → include_score()

The bug manifests only when \gregorioscore or \gabcsnippet is used inside an \input-ted file and no cached .gtex exists (clean-state build). Observed on NixOS sandboxes and GitHub Actions.

Typical symptoms:

  • ! Misplaced \noalign. / ! Undefined control sequence. \gre (hard error)
  • Silent content duplication in the PDF (scores or paragraphs appear twice, no error)

Fix

io.popen() invokes /bin/sh -c, which manages its own fd space before the final exec, eliminating the inheritance. Arguments are individually POSIX-single-quoted by gre_shellquote() so paths with spaces or shell metacharacters remain safe.

Closes #1757

davidweichiang
davidweichiang approved these changes May 29, 2026
View reviewed changes
Comment thread tex/gregoriotex.lua Outdated
Comment thread tex/gregoriotex.lua Outdated
lbssousa added a commit to lbssousa/gregorio that referenced this pull request May 29, 2026
@lbssousa @claude
refactor(lua): extract gre_exec helper, fix comment wording
71ffc9c 
Address review feedback on PR gregorio-project#1759:

- The f_pos corruption silently rewinds the read offset (not "advances");
  fix the comment to match the observed behaviour.
- Extract gre_exec(cmd) to eliminate the duplicated io.popen block that
  existed in both get_prog_output and compile_gabc.  gre_exec returns 0
  on success, a positive integer on failure, or nil when io.popen could
  not open a handle (nil is the sentinel compile_gabc uses to report that
  shell-escape may not be activated).  get_prog_output maps nil → 1 with
  the "or 1" idiom, preserving its existing rc semantics.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@davidweichiang

davidweichiang commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

I think this just needs an entry in CHANGELOG.md and then it looks good to me.

lbssousa added a commit to lbssousa/gregorio that referenced this pull request Jun 27, 2026
@lbssousa @claude
refactor(lua): extract gre_exec helper, fix comment wording
3890e08 
Address review feedback on PR gregorio-project#1759:

- The f_pos corruption silently rewinds the read offset (not "advances");
  fix the comment to match the observed behaviour.
- Extract gre_exec(cmd) to eliminate the duplicated io.popen block that
  existed in both get_prog_output and compile_gabc.  gre_exec returns 0
  on success, a positive integer on failure, or nil when io.popen could
  not open a handle (nil is the sentinel compile_gabc uses to report that
  shell-escape may not be activated).  get_prog_output maps nil → 1 with
  the "or 1" idiom, preserving its existing rc semantics.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rpspringuel

rpspringuel commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

It looks to me like it should be possible to generate a test file that follows your test plan and add it to the test repository. @lbssousa, can you please do that?

@lbssousa lbssousa mentioned this pull request Jul 8, 2026
Open
@lbssousa
lbssousa marked this pull request as draft July 11, 2026 18:34
@lbssousa lbssousa closed this Jul 11, 2026
@lbssousa
lbssousa deleted the fix-issue-1757 branch July 11, 2026 18:42
@lbssousa
lbssousa restored the fix-issue-1757 branch July 11, 2026 18:43
@davidweichiang

davidweichiang commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

@lbssousa Did you mean to close the other one (#1763) instead? I thought this one was still needed.

@lbssousa lbssousa reopened this Jul 11, 2026
@lbssousa

lbssousa commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

I'm reviewing the code for this PR. I thought about opening a new one, but on second thought, I decided to just force push to this one instead.

@lbssousa
lbssousa marked this pull request as ready for review July 11, 2026 20:32
@lbssousa
Fix fd-inheritance bug in gregoriotex.lua that corrupted \input state…
672b980 
… (issue gregorio-project#1757)

On Unix, os.spawn() leaks open file descriptors to the child process,
which can silently rewind the read offset of a \input-ted .tex file
that LuaTeX is still scanning. Route gabc/score compilation through
io.popen() instead, which doesn't exhibit this behavior. Windows is
unaffected and keeps using os.spawn().
lbssousa added a commit to lbssousa/gregorio that referenced this pull request Jul 12, 2026
@lbssousa @claude
refactor(lua): extract gre_exec helper, fix comment wording
9de7db9 
Address review feedback on PR gregorio-project#1759:

- The f_pos corruption silently rewinds the read offset (not "advances");
  fix the comment to match the observed behaviour.
- Extract gre_exec(cmd) to eliminate the duplicated io.popen block that
  existed in both get_prog_output and compile_gabc.  gre_exec returns 0
  on success, a positive integer on failure, or nil when io.popen could
  not open a handle (nil is the sentinel compile_gabc uses to report that
  shell-escape may not be activated).  get_prog_output maps nil → 1 with
  the "or 1" idiom, preserving its existing rc semantics.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lbssousa

lbssousa commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Just smoothed out the rough edges here. Could you please review it once again?

@davidweichiang

davidweichiang commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

What changed from before? (I can't tell because of the force-push.)

I haven't tested, but it looks reasonable to me.

@lbssousa

lbssousa commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

What changed from before? (I can't tell because of the force-push.)

I haven't tested, but it looks reasonable to me.

Just removed a local variable in favour of inline if-condition in gre_exec, and added better comments for auxiliary functions gre_shellquote and gre_build_cmdstr. This PR core remains unchanged.

I've also refactored the corredponding test gregorio-project/gregorio-test#426 from scripted to tex-output, as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidweichiang davidweichiang davidweichiang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

os.spawn fd-inheritance corrupts LuaTeX \input state in \gregorioscore and \gabcsnippet (triggered on clean builds)

3 participants

@lbssousa @davidweichiang @rpspringuel