1
Fork 0
mirror of https://github.com/RGBCube/serenity synced 2025-07-25 20:17:44 +00:00

Kernel: Clarify ambiguous {File,Description}::absolute_path

Found due to smelly code in InodeFile::absolute_path.

In particular, this replaces the following misleading methods:

File::absolute_path
This method *never* returns an actual path, and if called on an
InodeFile (which is impossible), it would VERIFY_NOT_REACHED().

OpenFileDescription::try_serialize_absolute_path
OpenFileDescription::absolute_path
These methods do not guarantee to return an actual path (just like the
other method), and just like Custody::absolute_path they do not
guarantee accuracy. In particular, just renaming the method made a
TOCTOU bug obvious.

The new method signatures use KResultOr, just like
try_serialize_absolute_path() already did.
This commit is contained in:
Ben Wiederhake 2021-10-30 00:45:23 +02:00 committed by Andreas Kling
parent 88ca12f037
commit c05c5a7ff4
28 changed files with 83 additions and 65 deletions

View file

@ -30,4 +30,9 @@ KResultOr<Memory::Region*> AnonymousFile::mmap(Process& process, OpenFileDescrip
return process.address_space().allocate_region_with_vmobject(range, m_vmobject, offset, {}, prot, shared);
}
KResultOr<NonnullOwnPtr<KString>> AnonymousFile::pseudo_path(const OpenFileDescription&) const
{
return KString::try_create(":anonymous-file:"sv);
}
}

View file

@ -24,7 +24,7 @@ public:
private:
virtual StringView class_name() const override { return "AnonymousFile"sv; }
virtual String absolute_path(const OpenFileDescription&) const override { return ":anonymous-file:"; }
virtual KResultOr<NonnullOwnPtr<KString>> pseudo_path(const OpenFileDescription&) const;
virtual bool can_read(const OpenFileDescription&, size_t) const override { return false; }
virtual bool can_write(const OpenFileDescription&, size_t) const override { return false; }
virtual KResultOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override { return ENOTSUP; }

View file

@ -132,9 +132,9 @@ KResultOr<size_t> FIFO::write(OpenFileDescription& fd, u64, const UserOrKernelBu
return m_buffer->write(buffer, size);
}
String FIFO::absolute_path(const OpenFileDescription&) const
KResultOr<NonnullOwnPtr<KString>> FIFO::pseudo_path(const OpenFileDescription&) const
{
return String::formatted("fifo:{}", m_fifo_id);
return KString::try_create(String::formatted("fifo:{}", m_fifo_id));
}
KResult FIFO::stat(::stat& st) const

View file

@ -45,7 +45,7 @@ private:
virtual KResult stat(::stat&) const override;
virtual bool can_read(const OpenFileDescription&, size_t) const override;
virtual bool can_write(const OpenFileDescription&, size_t) const override;
virtual String absolute_path(const OpenFileDescription&) const override;
virtual KResultOr<NonnullOwnPtr<KString>> pseudo_path(const OpenFileDescription&) const override;
virtual StringView class_name() const override { return "FIFO"sv; }
virtual bool is_fifo() const override { return true; }

View file

@ -93,7 +93,8 @@ public:
virtual KResultOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared);
virtual KResult stat(::stat&) const { return EBADF; }
virtual String absolute_path(const OpenFileDescription&) const = 0;
// Although this might be better described "name" or "description", these terms already have other meanings.
virtual KResultOr<NonnullOwnPtr<KString>> pseudo_path(const OpenFileDescription&) const = 0;
virtual KResult truncate(u64) { return EINVAL; }
virtual KResult sync() { return EINVAL; }

View file

@ -89,15 +89,14 @@ KResultOr<Memory::Region*> InodeFile::mmap(Process& process, OpenFileDescription
vmobject = TRY(Memory::SharedInodeVMObject::try_create_with_inode(inode()));
else
vmobject = TRY(Memory::PrivateInodeVMObject::try_create_with_inode(inode()));
auto path = TRY(description.try_serialize_absolute_path());
auto path = TRY(description.pseudo_path());
return process.address_space().allocate_region_with_vmobject(range, vmobject.release_nonnull(), offset, path->view(), prot, shared);
}
String InodeFile::absolute_path(const OpenFileDescription& description) const
KResultOr<NonnullOwnPtr<KString>> InodeFile::pseudo_path(const OpenFileDescription&) const
{
// If it has an inode, then it has a path, and therefore the caller should have been able to get a custody at some point.
VERIFY_NOT_REACHED();
VERIFY(description.custody());
return description.absolute_path();
}
KResult InodeFile::truncate(u64 size)

View file

@ -36,7 +36,7 @@ public:
virtual KResultOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
virtual KResult stat(::stat& buffer) const override { return inode().metadata().stat(buffer); }
virtual String absolute_path(const OpenFileDescription&) const override;
virtual KResultOr<NonnullOwnPtr<KString>> pseudo_path(const OpenFileDescription&) const override;
virtual KResult truncate(u64) override;
virtual KResult sync() override;

View file

@ -81,9 +81,9 @@ KResult InodeWatcher::close()
return KSuccess;
}
String InodeWatcher::absolute_path(const OpenFileDescription&) const
KResultOr<NonnullOwnPtr<KString>> InodeWatcher::pseudo_path(const OpenFileDescription&) const
{
return String::formatted("InodeWatcher:({})", m_wd_to_watches.size());
return KString::try_create(String::formatted("InodeWatcher:({})", m_wd_to_watches.size()));
}
void InodeWatcher::notify_inode_event(Badge<Inode>, InodeIdentifier inode_id, InodeWatcherEvent::Type event_type, String const& name)

View file

@ -50,7 +50,7 @@ public:
virtual KResultOr<size_t> write(OpenFileDescription&, u64, const UserOrKernelBuffer&, size_t) override { return EIO; }
virtual KResult close() override;
virtual String absolute_path(const OpenFileDescription&) const override;
virtual KResultOr<NonnullOwnPtr<KString>> pseudo_path(const OpenFileDescription&) const override;
virtual StringView class_name() const override { return "InodeWatcher"sv; };
virtual bool is_inode_watcher() const override { return true; }

View file

@ -349,19 +349,18 @@ KResult OpenFileDescription::close()
return m_file->close();
}
KResultOr<NonnullOwnPtr<KString>> OpenFileDescription::try_serialize_absolute_path()
KResultOr<NonnullOwnPtr<KString>> OpenFileDescription::original_absolute_path() const
{
if (!m_custody)
return ENOENT;
return m_custody->try_serialize_absolute_path();
}
KResultOr<NonnullOwnPtr<KString>> OpenFileDescription::pseudo_path() const
{
if (m_custody)
return m_custody->try_serialize_absolute_path();
// FIXME: Don't go through a String here!
return KString::try_create(m_file->absolute_path(*this));
}
String OpenFileDescription::absolute_path() const
{
if (m_custody)
return m_custody->absolute_path();
return m_file->absolute_path(*this);
return m_file->pseudo_path(*this);
}
InodeMetadata OpenFileDescription::metadata() const

View file

@ -64,8 +64,8 @@ public:
KResultOr<NonnullOwnPtr<KBuffer>> read_entire_file();
KResultOr<NonnullOwnPtr<KString>> try_serialize_absolute_path();
String absolute_path() const;
KResultOr<NonnullOwnPtr<KString>> original_absolute_path() const;
KResultOr<NonnullOwnPtr<KString>> pseudo_path() const;
bool is_direct() const { return m_direct; }

View file

@ -124,7 +124,8 @@ KResult VirtualFileSystem::mount_root(FileSystem& fs)
}
m_root_inode = root_inode;
dmesgln("VirtualFileSystem: mounted root from {} ({})", fs.class_name(), static_cast<FileBackedFileSystem&>(fs).file_description().absolute_path());
auto pseudo_path = TRY(static_cast<FileBackedFileSystem&>(fs).file_description().pseudo_path());
dmesgln("VirtualFileSystem: mounted root from {} ({})", fs.class_name(), pseudo_path);
m_mounts.with_exclusive([&](auto& mounts) {
mounts.append(move(mount));