Commit 94fcd171 authored by Paul Eggert's avatar Paul Eggert

Fix some fd issues when running subprocesses.

Fix bugs that can leak files or file descriptors on errors.
Don't unlink open temp files, as that's hard for users to diagnose
when things go awry (e.g., temp disk exhausted).
Don't bother to lock temp files.  Check for invalid recursion.
* callproc.c (synch_process_fd): Remove.  All uses removed.
(synch_process_tempfile): New var or macro.
(CALLPROC_STDOUT, CALLPROC_STDERR, CALLPROC_PIPEREAD, CALLPROC_FDS):
New constants.
(record_kill_process): New arg, the temp name.  All callers changed.
(delete_temp_file): Now just a simple wrapper around unlink.
(call_process_kill): New arg, the call_process_fd array.
Close them all.  Clear synch_process_pid.  Remove the temp file,
or arrange for it to be removed.
(call_process_cleanup) [MSDOS]: Arg no longer contains file name;
that's been moved to synch_process_tempfile.  Caller changed.
Do not remove the tempfile; that's now call_process_kill's
responsibility.
(call_process_cleanup) [!MSDOS]: Do not record unwind-protect for
call_process_kill; the caller now does that.
(call_process_cleanup): Do not close the process fd; that's now
call_process_kill's responsibility.
(Fcall_process): Implement via new function call_process, which
has most of the old body of Fcall_process, but with a different API.
(call_process): New function that does not open or close filefd if
it is nonnegative.  Record which fds need to be closed, and let
call_process_kill close (and remove the tempfile, on MSDOS) on error.
Signal an error if invoked recursively (could be done via a hook).
Simplify creation of the tempfile in the MSDOS case.
Don't create the output file until after checking for the executable.
Report any failure to open /dev/null.
Don't open /dev/null for writing twice; once is enough.
Don't create pipe if all output is being discarded or sent to file.
Don't worry about setting up the coding system or reading from the
pipe if all output is being discarded.
Hoist fd_error local into top level, to lessen block nesting.
Don't record deleted pid here; now done by Fcall_process_region.
(Fcall_process) [MSDOS]: Report mktemp failure immediately,
and note its success in synch_process_tempfile.
Do not leak resources when child_setup fails.
(Fcall_process) [!MSDOS && !WINDOWSNT]: Remove duplicate assignment
to child_errno.  Remove unnecessary close of fd0; it's close-on-exec.
(create_temp_file): Now returns open fd, with an additional
Lisp_Object * argument to return the name.  All callers changed.
Do not close the file; rewind it instead, and leave it open for
the caller.  Do not lock the temp file.  Unwind-protect the file
and the file-descriptor.
(Fcall_process_region): If the input is /dev/null, unwind-protect it.
If an asynchrounous process, record it here, not in call_process.
(syms_of_callproc) [MSDOS]: Initialize synch_process_tempfile.
* eval.c (set_unwind_protect): New function.
* fileio.c (write_region): New function, generalized from the
old Fwrite_region.  Do not lock temp files.
(Fwrite_region): Use it.
* lisp.h (set_unwind_protect, write_region): New decls.
* process.c: Include <verify.h>.
(make_process): Mark fds as initially closed.
(deleted_pid_list): Now a list of pid-filename pairs.
All uses changed.
(close_process_fd): New function.
(SUBPROCESS_STDIN, WRITE_TO_SUBPROCESS, READ_FROM_SUBPROCESS)
(SUBPROCESS_STDOUT, READ_FROM_EXEC_MONITOR, EXEC_MONITOR_OUTPUT):
New constants.  Verify that their number matches PROCESS_OPEN_FDS.
(create_process, create_pty, Fmake_serial_process)
(server_accept_connection): Record which fds need to be closed,
and let deactivate_process close them.
(Fmake_network_process): Do not discard the unwind-protect
until it's safe to do so.
(deactivate_process): Close the fds opened by create_process etc.
(Fprocess_send_eof): Adjust to new way of recording open fds.
Report an error if /dev/null can't be opened, instead of aborting.
* process.h (PROCESS_OPEN_FDS): New constant.
(struct Lisp_Process): New member open_fds.
(record_kill_process, record_deleted_pid): Adjust signatures.
(record_deleted_pid): Move decl here ...
* syswait.h (record_deleted_pid): ... from here.

Fixes: debbugs:15035
parent 4750fd7b
2013-08-12 Paul Eggert <eggert@cs.ucla.edu>
Fix some fd issues when running subprocesses (Bug#15035).
Fix bugs that can leak files or file descriptors on errors.
Don't unlink open temp files, as that's hard for users to diagnose
when things go awry (e.g., temp disk exhausted).
Don't bother to lock temp files. Check for invalid recursion.
* callproc.c (synch_process_fd): Remove. All uses removed.
(synch_process_tempfile): New var or macro.
(CALLPROC_STDOUT, CALLPROC_STDERR, CALLPROC_PIPEREAD, CALLPROC_FDS):
New constants.
(record_kill_process): New arg, the temp name. All callers changed.
(delete_temp_file): Now just a simple wrapper around unlink.
(call_process_kill): New arg, the call_process_fd array.
Close them all. Clear synch_process_pid. Remove the temp file,
or arrange for it to be removed.
(call_process_cleanup) [MSDOS]: Arg no longer contains file name;
that's been moved to synch_process_tempfile. Caller changed.
Do not remove the tempfile; that's now call_process_kill's
responsibility.
(call_process_cleanup) [!MSDOS]: Do not record unwind-protect for
call_process_kill; the caller now does that.
(call_process_cleanup): Do not close the process fd; that's now
call_process_kill's responsibility.
(Fcall_process): Implement via new function call_process, which
has most of the old body of Fcall_process, but with a different API.
(call_process): New function that does not open or close filefd if
it is nonnegative. Record which fds need to be closed, and let
call_process_kill close (and remove the tempfile, on MSDOS) on error.
Signal an error if invoked recursively (could be done via a hook).
Simplify creation of the tempfile in the MSDOS case.
Don't create the output file until after checking for the executable.
Report any failure to open /dev/null.
Don't open /dev/null for writing twice; once is enough.
Don't create pipe if all output is being discarded or sent to file.
Don't worry about setting up the coding system or reading from the
pipe if all output is being discarded.
Hoist fd_error local into top level, to lessen block nesting.
Don't record deleted pid here; now done by Fcall_process_region.
(Fcall_process) [MSDOS]: Report mktemp failure immediately,
and note its success in synch_process_tempfile.
Do not leak resources when child_setup fails.
(Fcall_process) [!MSDOS && !WINDOWSNT]: Remove duplicate assignment
to child_errno. Remove unnecessary close of fd0; it's close-on-exec.
(create_temp_file): Now returns open fd, with an additional
Lisp_Object * argument to return the name. All callers changed.
Do not close the file; rewind it instead, and leave it open for
the caller. Do not lock the temp file. Unwind-protect the file
and the file-descriptor.
(Fcall_process_region): If the input is /dev/null, unwind-protect it.
If an asynchrounous process, record it here, not in call_process.
(syms_of_callproc) [MSDOS]: Initialize synch_process_tempfile.
* eval.c (set_unwind_protect): New function.
* fileio.c (write_region): New function, generalized from the
old Fwrite_region. Do not lock temp files.
(Fwrite_region): Use it.
* lisp.h (set_unwind_protect, write_region): New decls.
* process.c: Include <verify.h>.
(make_process): Mark fds as initially closed.
(deleted_pid_list): Now a list of pid-filename pairs.
All uses changed.
(close_process_fd): New function.
(SUBPROCESS_STDIN, WRITE_TO_SUBPROCESS, READ_FROM_SUBPROCESS)
(SUBPROCESS_STDOUT, READ_FROM_EXEC_MONITOR, EXEC_MONITOR_OUTPUT):
New constants. Verify that their number matches PROCESS_OPEN_FDS.
(create_process, create_pty, Fmake_serial_process)
(server_accept_connection): Record which fds need to be closed,
and let deactivate_process close them.
(Fmake_network_process): Do not discard the unwind-protect
until it's safe to do so.
(deactivate_process): Close the fds opened by create_process etc.
(Fprocess_send_eof): Adjust to new way of recording open fds.
Report an error if /dev/null can't be opened, instead of aborting.
* process.h (PROCESS_OPEN_FDS): New constant.
(struct Lisp_Process): New member open_fds.
(record_kill_process, record_deleted_pid): Adjust signatures.
(record_deleted_pid): Move decl here ...
* syswait.h (record_deleted_pid): ... from here.
2013-08-11 Paul Eggert <eggert@cs.ucla.edu>
* decompress.c: Fix bugs with large buffers and weird inputs.
......
This diff is collapsed.
......@@ -3301,6 +3301,16 @@ clear_unwind_protect (ptrdiff_t count)
It need not be at the top of the stack. Discard the entry's
previous value without invoking it. */
void
set_unwind_protect (ptrdiff_t count, void (*func) (Lisp_Object),
Lisp_Object arg)
{
union specbinding *p = specpdl + count;
p->unwind.kind = SPECPDL_UNWIND;
p->unwind.func = func;
p->unwind.arg = arg;
}
void
set_unwind_protect_ptr (ptrdiff_t count, void (*func) (void *), void *arg)
{
......
......@@ -4746,25 +4746,39 @@ This does code conversion according to the value of
This calls `write-region-annotate-functions' at the start, and
`write-region-post-annotation-function' at the end. */)
(Lisp_Object start, Lisp_Object end, Lisp_Object filename, Lisp_Object append, Lisp_Object visit, Lisp_Object lockname, Lisp_Object mustbenew)
(Lisp_Object start, Lisp_Object end, Lisp_Object filename, Lisp_Object append,
Lisp_Object visit, Lisp_Object lockname, Lisp_Object mustbenew)
{
return write_region (start, end, filename, append, visit, lockname, mustbenew,
-1);
}
/* Like Fwrite_region, except that if DESC is nonnegative, it is a file
descriptor for FILENAME, so do not open or close FILENAME. */
Lisp_Object
write_region (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
Lisp_Object append, Lisp_Object visit, Lisp_Object lockname,
Lisp_Object mustbenew, int desc)
{
int desc;
int open_flags;
int mode;
off_t offset IF_LINT (= 0);
bool open_and_close_file = desc < 0;
bool ok;
int save_errno = 0;
const char *fn;
struct stat st;
EMACS_TIME modtime;
ptrdiff_t count = SPECPDL_INDEX ();
ptrdiff_t count1;
ptrdiff_t count1 IF_LINT (= 0);
Lisp_Object handler;
Lisp_Object visit_file;
Lisp_Object annotations;
Lisp_Object encoded_filename;
bool visiting = (EQ (visit, Qt) || STRINGP (visit));
bool quietly = !NILP (visit);
bool file_locked = 0;
struct gcpro gcpro1, gcpro2, gcpro3, gcpro4, gcpro5;
struct buffer *given_buffer;
struct coding_system coding;
......@@ -4832,7 +4846,6 @@ This calls `write-region-annotate-functions' at the start, and
record_unwind_protect (build_annotations_unwind,
Vwrite_region_annotation_buffers);
Vwrite_region_annotation_buffers = list1 (Fcurrent_buffer ());
count1 = SPECPDL_INDEX ();
given_buffer = current_buffer;
......@@ -4871,8 +4884,11 @@ This calls `write-region-annotate-functions' at the start, and
coding.mode |= CODING_MODE_SELECTIVE_DISPLAY;
#ifdef CLASH_DETECTION
if (!auto_saving)
lock_file (lockname);
if (open_and_close_file && !auto_saving)
{
lock_file (lockname);
file_locked = 1;
}
#endif /* CLASH_DETECTION */
encoded_filename = ENCODE_FILE (filename);
......@@ -4889,19 +4905,23 @@ This calls `write-region-annotate-functions' at the start, and
mode = auto_saving ? auto_save_mode_bits : 0666;
#endif
desc = emacs_open (fn, open_flags, mode);
if (desc < 0)
if (open_and_close_file)
{
int open_errno = errno;
desc = emacs_open (fn, open_flags, mode);
if (desc < 0)
{
int open_errno = errno;
#ifdef CLASH_DETECTION
if (!auto_saving) unlock_file (lockname);
if (file_locked)
unlock_file (lockname);
#endif /* CLASH_DETECTION */
UNGCPRO;
report_file_errno ("Opening output file", filename, open_errno);
}
UNGCPRO;
report_file_errno ("Opening output file", filename, open_errno);
}
record_unwind_protect_int (close_file_unwind, desc);
count1 = SPECPDL_INDEX ();
record_unwind_protect_int (close_file_unwind, desc);
}
if (NUMBERP (append))
{
......@@ -4910,7 +4930,8 @@ This calls `write-region-annotate-functions' at the start, and
{
int lseek_errno = errno;
#ifdef CLASH_DETECTION
if (!auto_saving) unlock_file (lockname);
if (file_locked)
unlock_file (lockname);
#endif /* CLASH_DETECTION */
UNGCPRO;
report_file_errno ("Lseek error", filename, lseek_errno);
......@@ -4945,9 +4966,9 @@ This calls `write-region-annotate-functions' at the start, and
immediate_quit = 0;
/* fsync is not crucial for auto-save files, since they might lose
some work anyway. */
if (!auto_saving && !write_region_inhibit_fsync)
/* fsync is not crucial for temporary files. Nor for auto-save
files, since they might lose some work anyway. */
if (open_and_close_file && !auto_saving && !write_region_inhibit_fsync)
{
/* Transfer data and metadata to disk, retrying if interrupted.
fsync can report a write failure here, e.g., due to disk full
......@@ -4971,12 +4992,15 @@ This calls `write-region-annotate-functions' at the start, and
ok = 0, save_errno = errno;
}
/* NFS can report a write failure now. */
if (emacs_close (desc) < 0)
ok = 0, save_errno = errno;
if (open_and_close_file)
{
/* NFS can report a write failure now. */
if (emacs_close (desc) < 0)
ok = 0, save_errno = errno;
/* Discard the unwind protect for close_file_unwind. */
specpdl_ptr = specpdl + count1;
/* Discard the unwind protect for close_file_unwind. */
specpdl_ptr = specpdl + count1;
}
/* Some file systems have a bug where st_mtime is not updated
properly after a write. For example, CIFS might not see the
......@@ -5052,7 +5076,7 @@ This calls `write-region-annotate-functions' at the start, and
unbind_to (count, Qnil);
#ifdef CLASH_DETECTION
if (!auto_saving)
if (file_locked)
unlock_file (lockname);
#endif /* CLASH_DETECTION */
......
......@@ -3746,11 +3746,12 @@ extern Lisp_Object internal_condition_case_n
Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *));
extern void specbind (Lisp_Object, Lisp_Object);
extern void record_unwind_protect (void (*) (Lisp_Object), Lisp_Object);
extern void record_unwind_protect_int (void (*) (int), int);
extern void record_unwind_protect_ptr (void (*) (void *), void *);
extern void record_unwind_protect_int (void (*) (int), int);
extern void record_unwind_protect_void (void (*) (void));
extern void record_unwind_protect_nothing (void);
extern void clear_unwind_protect (ptrdiff_t);
extern void set_unwind_protect (ptrdiff_t, void (*) (Lisp_Object), Lisp_Object);
extern void set_unwind_protect_ptr (ptrdiff_t, void (*) (void *), void *);
extern Lisp_Object unbind_to (ptrdiff_t, Lisp_Object);
extern _Noreturn void error (const char *, ...) ATTRIBUTE_FORMAT_PRINTF (1, 2);
......@@ -3830,6 +3831,9 @@ extern Lisp_Object Qfile_directory_p;
extern Lisp_Object Qinsert_file_contents;
extern Lisp_Object Qfile_name_history;
extern Lisp_Object expand_and_dir_to_file (Lisp_Object, Lisp_Object);
extern Lisp_Object write_region (Lisp_Object, Lisp_Object, Lisp_Object,
Lisp_Object, Lisp_Object, Lisp_Object,
Lisp_Object, int);
EXFUN (Fread_file_name, 6); /* Not a normal DEFUN. */
extern void close_file_unwind (int);
extern void fclose_unwind (void *);
......
This diff is collapsed.
......@@ -31,6 +31,11 @@ INLINE_HEADER_BEGIN
# define PROCESS_INLINE INLINE
#endif
/* Bound on number of file descriptors opened on behalf of a process,
that need to be closed. */
enum { PROCESS_OPEN_FDS = 6 };
/* This structure records information about a subprocess
or network connection. */
......@@ -115,6 +120,9 @@ struct Lisp_Process
int infd;
/* Descriptor by which we write to this process */
int outfd;
/* Descriptors that were created for this process and that need
closing. Unused entries are negative. */
int open_fd[PROCESS_OPEN_FDS];
/* Event-count of last event in which this process changed status. */
EMACS_INT tick;
/* Event-count of last such event reported. */
......@@ -210,13 +218,16 @@ enum
extern void block_child_signal (void);
extern void unblock_child_signal (void);
extern void record_kill_process (struct Lisp_Process *);
extern void record_kill_process (struct Lisp_Process *, Lisp_Object);
/* Defined in process.c. */
/* Defined in sysdep.c. */
extern Lisp_Object list_system_processes (void);
extern Lisp_Object system_process_attributes (Lisp_Object);
/* Defined in process.c. */
extern void record_deleted_pid (pid_t, Lisp_Object);
extern void hold_keyboard_input (void);
extern void unhold_keyboard_input (void);
extern bool kbd_on_hold_p (void);
......
......@@ -52,9 +52,6 @@ along with GNU Emacs. If not, see <http://www.gnu.org/licenses/>. */
#define WTERMSIG(status) ((status) & 0x7f)
#endif
/* Defined in process.c. */
extern void record_deleted_pid (pid_t);
/* Defined in sysdep.c. */
extern void wait_for_termination (pid_t, int *, bool);
extern pid_t child_status_changed (pid_t, int *, int);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment