Skip to content

Commit

Permalink
Add close-on-exec flag for file descriptors.
Browse files Browse the repository at this point in the history
I was surprised to find that dup()'ed fds don't share the close-on-exec flag.
That means it has to be stored separately from the FileDescriptor object.
  • Loading branch information
awesomekling committed Nov 13, 2018
1 parent 19b9401 commit 97c7995
Show file tree
Hide file tree
Showing 10 changed files with 74 additions and 35 deletions.
11 changes: 11 additions & 0 deletions AK/Vector.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,17 @@ class Vector {
m_impl->remove(index);
}

Vector& operator=(const Vector<T>& other)
{
if (this != &other) {
clear();
ensureCapacity(other.size());
for (const auto& v : other)
unchecked_append(v);
}
return *this;
}

void append(Vector<T>&& other)
{
Vector<T> tmp = move(other);
Expand Down
66 changes: 40 additions & 26 deletions Kernel/Process.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,14 @@ int Process::do_exec(const String& path, Vector<String>&& arguments, Vector<Stri
memset(m_signal_action_data, 0, sizeof(m_signal_action_data));
m_signal_mask = 0xffffffff;

for (size_t i = 0; i < m_fds.size(); ++i) {
auto& daf = m_fds[i];
if (daf.descriptor && daf.flags & FD_CLOEXEC) {
daf.descriptor->close();
daf = { };
}
}

InterruptDisabler disabler;
Scheduler::prepare_to_modify_tss(*this);

Expand Down Expand Up @@ -597,21 +605,22 @@ Process::Process(String&& name, uid_t uid, gid_t gid, pid_t ppid, RingLevel ring
MM.populate_page_directory(*m_page_directory);

if (fork_parent) {
m_file_descriptors.resize(fork_parent->m_file_descriptors.size());
for (size_t i = 0; i < fork_parent->m_file_descriptors.size(); ++i) {
if (!fork_parent->m_file_descriptors[i])
m_fds.resize(fork_parent->m_fds.size());
for (size_t i = 0; i < fork_parent->m_fds.size(); ++i) {
if (!fork_parent->m_fds[i].descriptor)
continue;
#ifdef FORK_DEBUG
dbgprintf("fork: cloning fd %u... (%p) istty? %u\n", i, fork_parent->m_file_descriptors[i].ptr(), fork_parent->m_file_descriptors[i]->isTTY());
dbgprintf("fork: cloning fd %u... (%p) istty? %u\n", i, fork_parent->m_fds[i].ptr(), fork_parent->m_fds[i]->isTTY());
#endif
m_file_descriptors[i] = fork_parent->m_file_descriptors[i]->clone();
m_fds[i].descriptor = fork_parent->m_fds[i].descriptor->clone();
m_fds[i].flags = fork_parent->m_fds[i].flags;
}
} else {
m_file_descriptors.resize(m_max_open_file_descriptors);
m_fds.resize(m_max_open_file_descriptors);
if (tty) {
m_file_descriptors[0] = tty->open(O_RDONLY);
m_file_descriptors[1] = tty->open(O_WRONLY);
m_file_descriptors[2] = tty->open(O_WRONLY);
m_fds[0].set(tty->open(O_RDONLY));
m_fds[1].set(tty->open(O_WRONLY));
m_fds[2].set(tty->open(O_WRONLY));
}
}

Expand Down Expand Up @@ -929,17 +938,17 @@ FileDescriptor* Process::file_descriptor(int fd)
{
if (fd < 0)
return nullptr;
if ((size_t)fd < m_file_descriptors.size())
return m_file_descriptors[fd].ptr();
if ((size_t)fd < m_fds.size())
return m_fds[fd].descriptor.ptr();
return nullptr;
}

const FileDescriptor* Process::file_descriptor(int fd) const
{
if (fd < 0)
return nullptr;
if ((size_t)fd < m_file_descriptors.size())
return m_file_descriptors[fd].ptr();
if ((size_t)fd < m_fds.size())
return m_fds[fd].descriptor.ptr();
return nullptr;
}

Expand Down Expand Up @@ -1063,7 +1072,7 @@ int Process::sys$close(int fd)
if (!descriptor)
return -EBADF;
int rc = descriptor->close();
m_file_descriptors[fd] = nullptr;
m_fds[fd] = { };
return rc;
}

Expand All @@ -1082,15 +1091,19 @@ int Process::sys$fcntl(int fd, int cmd, dword arg)
auto* descriptor = file_descriptor(fd);
if (!descriptor)
return -EBADF;
// NOTE: The FD flags are not shared between FileDescriptor objects.
// This means that dup() doesn't copy the FD_CLOEXEC flag!
switch (cmd) {
case F_GETFD:
return descriptor->fd_flags();
return m_fds[fd].flags;
case F_SETFD:
return descriptor->set_fd_flags(arg);
m_fds[fd].flags = arg;
break;
case F_GETFL:
return descriptor->file_flags();
case F_SETFL:
return descriptor->set_file_flags(arg);
descriptor->set_file_flags(arg);
break;
default:
ASSERT_NOT_REACHED();
}
Expand Down Expand Up @@ -1180,7 +1193,7 @@ int Process::sys$getcwd(char* buffer, size_t size)
size_t Process::number_of_open_file_descriptors() const
{
size_t count = 0;
for (auto& descriptor : m_file_descriptors) {
for (auto& descriptor : m_fds) {
if (descriptor)
++count;
}
Expand All @@ -1204,18 +1217,19 @@ int Process::sys$open(const char* path, int options)

int fd = 0;
for (; fd < (int)m_max_open_file_descriptors; ++fd) {
if (!m_file_descriptors[fd])
if (!m_fds[fd])
break;
}
m_file_descriptors[fd] = move(descriptor);
dword flags = (options & O_CLOEXEC) ? FD_CLOEXEC : 0;
m_fds[fd].set(move(descriptor), flags);
return fd;
}

int Process::alloc_fd()
{
int fd = -1;
for (int i = 0; i < (int)m_max_open_file_descriptors; ++i) {
if (!m_file_descriptors[i]) {
if (!m_fds[i]) {
fd = i;
break;
}
Expand All @@ -1231,11 +1245,11 @@ int Process::sys$pipe(int* pipefd)
auto fifo = FIFO::create();

int reader_fd = alloc_fd();
m_file_descriptors[reader_fd] = FileDescriptor::create_pipe_reader(*fifo);
m_fds[reader_fd].set(FileDescriptor::create_pipe_reader(*fifo));
pipefd[0] = reader_fd;

int writer_fd = alloc_fd();
m_file_descriptors[writer_fd] = FileDescriptor::create_pipe_writer(*fifo);
m_fds[writer_fd].set(FileDescriptor::create_pipe_writer(*fifo));
pipefd[1] = writer_fd;

return 0;
Expand Down Expand Up @@ -1616,10 +1630,10 @@ int Process::sys$dup(int old_fd)
return -EMFILE;
int new_fd = 0;
for (; new_fd < (int)m_max_open_file_descriptors; ++new_fd) {
if (!m_file_descriptors[new_fd])
if (!m_fds[new_fd])
break;
}
m_file_descriptors[new_fd] = descriptor;
m_fds[new_fd].set(descriptor);
return new_fd;
}

Expand All @@ -1630,7 +1644,7 @@ int Process::sys$dup2(int old_fd, int new_fd)
return -EBADF;
if (number_of_open_file_descriptors() == m_max_open_file_descriptors)
return -EMFILE;
m_file_descriptors[new_fd] = descriptor;
m_fds[new_fd].set(descriptor);
return new_fd;
}

Expand Down
9 changes: 8 additions & 1 deletion Kernel/Process.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,14 @@ class Process : public InlineLinkedListNode<Process> {
DWORD m_wakeupTime { 0 };
TSS32 m_tss;
TSS32 m_tss_to_resume_kernel;
Vector<RetainPtr<FileDescriptor>> m_file_descriptors;
struct FileDescriptorAndFlags {
operator bool() const { return !!descriptor; }
void clear() { descriptor = nullptr; flags = 0; }
void set(RetainPtr<FileDescriptor>&& d, dword f = 0) { descriptor = move(d), flags = f; }
RetainPtr<FileDescriptor> descriptor;
dword flags { 0 };
};
Vector<FileDescriptorAndFlags> m_fds;
RingLevel m_ring { Ring0 };
int m_error { 0 };
void* m_kernelStack { nullptr };
Expand Down
4 changes: 2 additions & 2 deletions Kernel/Scheduler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ bool Scheduler::pick_next()
if (process.state() == Process::BlockedRead) {
ASSERT(process.m_fdBlockedOnRead != -1);
// FIXME: Block until the amount of data wanted is available.
if (process.m_file_descriptors[process.m_fdBlockedOnRead]->hasDataAvailableForRead())
if (process.m_fds[process.m_fdBlockedOnRead].descriptor->hasDataAvailableForRead())
process.unblock();
return true;
}

if (process.state() == Process::BlockedWrite) {
ASSERT(process.m_blocked_fd != -1);
if (process.m_file_descriptors[process.m_blocked_fd]->can_write())
if (process.m_fds[process.m_blocked_fd].descriptor->can_write())
process.unblock();
return true;
}
Expand Down
2 changes: 2 additions & 0 deletions LibC/fcntl.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ __BEGIN_DECLS
#define F_GETFL 3
#define F_SETFL 4

#define FD_CLOEXEC 1

int fcntl(int fd, int cmd, ...);

__END_DECLS
1 change: 1 addition & 0 deletions LibC/unistd.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ int isatty(int fd);
#define O_NONBLOCK 04000
#define O_DIRECTORY 00200000
#define O_ 00400000
#define O_CLOEXEC 02000000

__END_DECLS

1 change: 0 additions & 1 deletion VirtualFileSystem/FileDescriptor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ RetainPtr<FileDescriptor> FileDescriptor::clone()
descriptor->m_currentOffset = m_currentOffset;
#ifdef SERENITY
descriptor->m_isBlocking = m_isBlocking;
descriptor->m_fd_flags = m_fd_flags;
descriptor->m_file_flags = m_file_flags;
#endif
return descriptor;
Expand Down
6 changes: 1 addition & 5 deletions VirtualFileSystem/FileDescriptor.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ class FileDescriptor : public Retainable<FileDescriptor> {
void setBlocking(bool b) { m_isBlocking = b; }

dword file_flags() const { return m_file_flags; }
int set_file_flags(dword flags) { m_file_flags = flags; return 0; }

dword fd_flags() const { return m_fd_flags; }
int set_fd_flags(dword flags) { m_fd_flags = flags; return 0; }
void set_file_flags(dword flags) { m_file_flags = flags; }

bool is_fifo() const { return m_fifo; }
FIFO::Direction fifo_direction() { return m_fifo_direction; }
Expand All @@ -77,7 +74,6 @@ class FileDescriptor : public Retainable<FileDescriptor> {

#ifdef SERENITY
bool m_isBlocking { true };
dword m_fd_flags { 0 };
dword m_file_flags { 0 };

RetainPtr<FIFO> m_fifo;
Expand Down
2 changes: 2 additions & 0 deletions VirtualFileSystem/UnixTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ namespace Unix {
#define F_GETFL 3
#define F_SETFL 4

#define FD_CLOEXEC 1

/* c_cc characters */
#define VINTR 0
#define VQUIT 1
Expand Down
7 changes: 7 additions & 0 deletions VirtualFileSystem/VirtualFileSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,15 @@
#define O_RDONLY 0
#define O_WRONLY 1
#define O_RDWR 2
#define O_CREAT 0100
#define O_EXCL 0200
#define O_NOCTTY 0400
#define O_TRUNC 01000
#define O_APPEND 02000
#define O_NONBLOCK 04000
#define O_DIRECTORY 00200000
#define O_ 00400000
#define O_CLOEXEC 02000000
#define O__NOERROR 0x4000000

class CharacterDevice;
Expand Down

0 comments on commit 97c7995

Please sign in to comment.