git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed

From
Johannes Sixt <j.sixt@viscovery.net>
Date
Aug 26, 2008, 06:33 UTC
Message-ID
<48B3A3CC.3060906@viscovery.net>
In-Reply-To
<7vbpzgb94q.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
Show 27 quoted lines
> Paolo Bonzini <bonzini@gnu.org> writes:
> 
>>  int start_command(struct child_process *cmd)
>>  {
>>  	int need_in, need_out, need_err;
>>  	int fdin[2], fdout[2], fderr[2];
>>  
>>  	/*
>> +	 * Make sure that all file descriptors <= 2 are open, otherwise we
>> +	 * mess them up when dup'ing pipes onto stdin/stdout/stderr.  Since
>> +	 * we are at it, save a file descriptor on /dev/null to use it later.
>> +	 */
>> +	if (devnull_fd == -1) {
>> +		devnull_fd = open("/dev/null", O_RDWR);
>> +		while (devnull_fd >= 0 && devnull_fd <= 2)
>> +			devnull_fd = dup(devnull_fd);
>> +		if (devnull_fd == -1)
>> +			die("opening /dev/null failed (%s)", strerror(errno));
>> +	}
>> +
> 
> I may be misreading the patch but, this logic always opens /dev/null, if
> nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and
> worse, it keeps fd=3 open.
> 
> Making sure low fds 0, 1 and 2 are open is a good thing.  I do not think
> clobbering fd=3 is good.

It is sometimes _unnecessary_, but I don't see why it should hurt. The effect on performance will be in the noise.

> Also shouldn't this be done only on the side that dup()s fds around,
> i.e. in the child process after fork()?  Why is this done for the parent?

Because it must be done *before* the pipe()s are created so that they don't occupy fds 0-2.

-- Hannes
Previous: Junio C HamanoNext: Paolo Bonzini
Message 10 of 37 in “Fix start_command() pipe bug when stdin is closed.”
  1. Fix start_command() pipe bug when stdin is closed.Karl Chen, Aug 25, 2008
  2. Johannes SixtAug 25, 2008
  3. Paolo BonziniAug 25, 2008
  4. fix start_command() bug when stdin is closedPaolo Bonzini, Aug 25, 2008
  5. Johannes SixtAug 25, 2008
  6. fix start_command() bug when stdin is closedPaolo Bonzini, Aug 25, 2008
  7. Karl ChenAug 25, 2008
  8. Junio C HamanoAug 26, 2008
  9. Junio C HamanoAug 26, 2008
  10. Johannes SixtAug 26, 2008
  11. Paolo BonziniAug 26, 2008
  12. be paranoid about closed stdin/stdout/stderrPaolo Bonzini, Aug 26, 2008
  13. Johannes SixtAug 26, 2008
  14. Stephen R. van den BergAug 26, 2008
  15. Avery PennarunAug 27, 2008
  16. Stephen R. van den BergAug 27, 2008
  17. Paolo BonziniAug 27, 2008
  18. make git-shell paranoid about closed stdin/stdout/stderrPaolo Bonzini, Aug 27, 2008
  19. Stephen R. van den BergAug 27, 2008
  20. Junio C HamanoAug 27, 2008
  21. Paolo BonziniAug 28, 2008
  22. Stephen R. van den BergAug 28, 2008
  23. Avery PennarunAug 27, 2008
  24. Nick AndrewAug 28, 2008
  25. Stephen R. van den BergAug 28, 2008
  26. Junio C HamanoAug 26, 2008
  27. Paolo BonziniAug 26, 2008
  28. Junio C HamanoAug 26, 2008
  29. Junio C HamanoAug 26, 2008
  30. Stephen R. van den BergAug 26, 2008
  31. Karl ChenAug 27, 2008
  32. Paolo BonziniAug 27, 2008
  33. Stephen R. van den BergAug 27, 2008
  34. Johannes SixtAug 27, 2008
  35. Paolo BonziniAug 27, 2008
  36. Nick AndrewAug 27, 2008
  37. Karl ChenAug 25, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.