hrev53100 adds 2 changesets to branch 'master'
old head: 1e4109cdb441e19780aed79fcd298f5cc6041a8f
new head: 097bbc7fd28bade702f21c9b4082ffefd0082a62
overview:
https://git.haiku-os.org/haiku/log/?qt=range&q=097bbc7fd28b+%5E1e4109cdb441
----------------------------------------------------------------------------
728b515e1c78: kernel: Bounds-check the parameters passed to file_cache_read().
cache_io() did some bounds checking, but for uncached reads it was
entirely on the filesystem. Now we are more consistent: do all the
bounds checking for reads in the file cache, and do all bounds
checking for writes in the filesystems.
This makes sense as nearly all file systems were doing the exact
same logic for read() but of course all have different logic
for write(), due to block allocation, etc.
This potentially fixes #14993.
Change-Id: Iaf3e549001344cf375c7b8de549fc169d77bdbb2
Reviewed-on: https://review.haiku-os.org/c/1420
Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>
097bbc7fd28b: file_systems: Remove now-redundant bounds checks before
file_cache_read().
Change-Id: Iafb7d188c7e7cb4406d924eb3354a7ede40c6641
Reviewed-on: https://review.haiku-os.org/c/1421
Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>
Reviewed-by: Michael Lotz <mmlr@xxxxxxxx>
[ Augustin Cavalier <waddlesplash@xxxxxxxxx> ]
----------------------------------------------------------------------------
8 files changed, 21 insertions(+), 96 deletions(-)
src/add-ons/kernel/file_systems/bfs/Inode.cpp | 15 ----------
src/add-ons/kernel/file_systems/exfat/Inode.cpp | 16 -----------
src/add-ons/kernel/file_systems/ext2/Inode.cpp | 16 -----------
src/add-ons/kernel/file_systems/fat/file.c | 12 --------
.../file_systems/iso9660/kernel_interface.cpp | 10 -------
.../packagefs/package/PackageFile.cpp | 6 ----
src/add-ons/kernel/file_systems/udf/Icb.cpp | 12 ++++----
src/system/kernel/cache/file_cache.cpp | 30 ++++++++++----------
############################################################################
Commit: 728b515e1c782a04b98f4361c76a8a21091eab16
URL: https://git.haiku-os.org/haiku/commit/?id=728b515e1c78
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Mon Apr 29 18:16:27 2019 UTC
Committer: waddlesplash <waddlesplash@xxxxxxxxx>
Commit-Date: Mon Apr 29 20:21:05 2019 UTC
Ticket: https://dev.haiku-os.org/ticket/14993
kernel: Bounds-check the parameters passed to file_cache_read().
cache_io() did some bounds checking, but for uncached reads it was
entirely on the filesystem. Now we are more consistent: do all the
bounds checking for reads in the file cache, and do all bounds
checking for writes in the filesystems.
This makes sense as nearly all file systems were doing the exact
same logic for read() but of course all have different logic
for write(), due to block allocation, etc.
This potentially fixes #14993.
Change-Id: Iaf3e549001344cf375c7b8de549fc169d77bdbb2
Reviewed-on: https://review.haiku-os.org/c/1420
Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>
----------------------------------------------------------------------------
diff --git a/src/system/kernel/cache/file_cache.cpp
b/src/system/kernel/cache/file_cache.cpp
index 64f81a8f91..5f1f459617 100644
--- a/src/system/kernel/cache/file_cache.cpp
+++ b/src/system/kernel/cache/file_cache.cpp
@@ -707,30 +707,15 @@ cache_io(void* _cacheRef, void* cookie, off_t offset,
addr_t buffer,
file_cache_ref* ref = (file_cache_ref*)_cacheRef;
VMCache* cache = ref->cache;
- off_t fileSize = cache->virtual_end;
bool useBuffer = buffer != 0;
TRACE(("cache_io(ref = %p, offset = %Ld, buffer = %p, size = %lu,
%s)\n",
ref, offset, (void*)buffer, *_size, doWrite ? "write" :
"read"));
- // out of bounds access?
- if (offset >= fileSize || offset < 0) {
- *_size = 0;
- return B_OK;
- }
-
int32 pageOffset = offset & (B_PAGE_SIZE - 1);
size_t size = *_size;
offset -= pageOffset;
- if ((off_t)(offset + pageOffset + size) > fileSize) {
- // adapt size to be within the file's offsets
- size = fileSize - pageOffset - offset;
- *_size = size;
- }
- if (size == 0)
- return B_OK;
-
// "offset" and "lastOffset" are always aligned to B_PAGE_SIZE,
// the "last*" variables always point to the end of the last
// satisfied request part
@@ -1287,6 +1272,17 @@ file_cache_read(void* _cacheRef, void* cookie, off_t
offset, void* buffer,
TRACE(("file_cache_read(ref = %p, offset = %Ld, buffer = %p, size =
%lu)\n",
ref, offset, buffer, *_size));
+ // Bounds checking. We do this here so it applies to uncached I/O.
+ if (offset < 0)
+ return B_BAD_VALUE;
+ const off_t fileSize = ref->cache->virtual_end;
+ if (offset >= fileSize || *_size == 0) {
+ *_size = 0;
+ return B_OK;
+ }
+ if ((off_t)(offset + *_size) > fileSize)
+ *_size = fileSize - offset;
+
if (ref->disabled_count > 0) {
// Caching is disabled -- read directly from the file.
generic_io_vec vec;
@@ -1308,6 +1304,10 @@ file_cache_write(void* _cacheRef, void* cookie, off_t
offset,
{
file_cache_ref* ref = (file_cache_ref*)_cacheRef;
+ // We don't do bounds checking here, as we are relying on the
+ // file system which called us to already have done that and made
+ // adjustments as necessary, unlike in read().
+
if (ref->disabled_count > 0) {
// Caching is disabled -- write directly to the file.
############################################################################
Revision: hrev53100
Commit: 097bbc7fd28bade702f21c9b4082ffefd0082a62
URL: https://git.haiku-os.org/haiku/commit/?id=097bbc7fd28b
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Mon Apr 29 18:19:04 2019 UTC
Committer: waddlesplash <waddlesplash@xxxxxxxxx>
Commit-Date: Mon Apr 29 20:21:05 2019 UTC
file_systems: Remove now-redundant bounds checks before file_cache_read().
Change-Id: Iafb7d188c7e7cb4406d924eb3354a7ede40c6641
Reviewed-on: https://review.haiku-os.org/c/1421
Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>
Reviewed-by: Michael Lotz <mmlr@xxxxxxxx>
----------------------------------------------------------------------------
diff --git a/src/add-ons/kernel/file_systems/bfs/Inode.cpp
b/src/add-ons/kernel/file_systems/bfs/Inode.cpp
index 134e05ce09..2fe7a8f212 100644
--- a/src/add-ons/kernel/file_systems/bfs/Inode.cpp
+++ b/src/add-ons/kernel/file_systems/bfs/Inode.cpp
@@ -1556,21 +1556,6 @@ Inode::FindBlockRun(off_t pos, block_run& run, off_t&
offset)
status_t
Inode::ReadAt(off_t pos, uint8* buffer, size_t* _length)
{
- size_t length = *_length;
-
- // set/check boundaries for pos/length
- if (pos < 0)
- return B_BAD_VALUE;
-
- InodeReadLocker locker(this);
-
- if (pos >= Size() || length == 0) {
- *_length = 0;
- return B_NO_ERROR;
- }
-
- locker.Unlock();
-
return file_cache_read(FileCache(), NULL, pos, buffer, _length);
}
diff --git a/src/add-ons/kernel/file_systems/exfat/Inode.cpp
b/src/add-ons/kernel/file_systems/exfat/Inode.cpp
index f7c1d405af..279d57b14c 100644
--- a/src/add-ons/kernel/file_systems/exfat/Inode.cpp
+++ b/src/add-ons/kernel/file_systems/exfat/Inode.cpp
@@ -173,22 +173,6 @@ Inode::FindBlock(off_t pos, off_t& physical, off_t
*_length)
status_t
Inode::ReadAt(off_t pos, uint8* buffer, size_t* _length)
{
- size_t length = *_length;
-
- // set/check boundaries for pos/length
- if (pos < 0) {
- ERROR("inode %" B_PRIdINO ": ReadAt failed(pos %" B_PRIdOFF",
length %"
- B_PRIuSIZE ")\n", ID(), pos, length);
- return B_BAD_VALUE;
- }
-
- if (pos >= Size() || length == 0) {
- TRACE("inode %" B_PRIdINO ": ReadAt 0 (pos %" B_PRIdOFF",
length %"
- B_PRIuSIZE ")\n", ID(), pos, length);
- *_length = 0;
- return B_NO_ERROR;
- }
-
return file_cache_read(FileCache(), NULL, pos, buffer, _length);
}
diff --git a/src/add-ons/kernel/file_systems/ext2/Inode.cpp
b/src/add-ons/kernel/file_systems/ext2/Inode.cpp
index 19d0251240..95eb2011a0 100644
--- a/src/add-ons/kernel/file_systems/ext2/Inode.cpp
+++ b/src/add-ons/kernel/file_systems/ext2/Inode.cpp
@@ -206,22 +206,6 @@ Inode::FindBlock(off_t offset, fsblock_t& block, uint32
*_count)
status_t
Inode::ReadAt(off_t pos, uint8* buffer, size_t* _length)
{
- size_t length = *_length;
-
- // set/check boundaries for pos/length
- if (pos < 0) {
- ERROR("inode %" B_PRIdINO ": ReadAt failed(pos %" B_PRIdOFF
- ", length %" B_PRIuSIZE ")\n", ID(), pos, length);
- return B_BAD_VALUE;
- }
-
- if (pos >= Size() || length == 0) {
- TRACE("inode %" B_PRIdINO ": ReadAt 0 (pos %" B_PRIdOFF ",
length %"
- B_PRIuSIZE ")\n", ID(), pos, length);
- *_length = 0;
- return B_NO_ERROR;
- }
-
return file_cache_read(FileCache(), NULL, pos, buffer, _length);
}
diff --git a/src/add-ons/kernel/file_systems/fat/file.c
b/src/add-ons/kernel/file_systems/fat/file.c
index 749750213b..be82ba905c 100644
--- a/src/add-ons/kernel/file_systems/fat/file.c
+++ b/src/add-ons/kernel/file_systems/fat/file.c
@@ -370,20 +370,8 @@ dosfs_read(fs_volume *_vol, fs_vnode *_node, void
*_cookie, off_t pos,
DPRINTF(0, ("dosfs_read called %" B_PRIuSIZE " bytes at %" B_PRIdOFF
" (vnode id %" B_PRIdINO ")\n", *len, pos, node->vnid));
- if (pos < 0) pos = 0;
-
- if ((node->st_size == 0) || (*len == 0) || (pos >= node->st_size)) {
- *len = 0;
- goto bi;
- }
-
- // truncate bytes to read to file size
- if (pos + *len >= node->st_size)
- *len = node->st_size - pos;
-
result = file_cache_read(node->cache, cookie, pos, buf, len);
-bi:
if (result != B_OK) {
DPRINTF(0, ("dosfs_read (%s)\n", strerror(result)));
} else {
diff --git a/src/add-ons/kernel/file_systems/iso9660/kernel_interface.cpp
b/src/add-ons/kernel/file_systems/iso9660/kernel_interface.cpp
index e5d46b2981..0a9bd987b4 100644
--- a/src/add-ons/kernel/file_systems/iso9660/kernel_interface.cpp
+++ b/src/add-ons/kernel/file_systems/iso9660/kernel_interface.cpp
@@ -528,16 +528,6 @@ fs_read(fs_volume* _volume, fs_vnode* _node, void* cookie,
off_t pos,
if ((node->flags & ISO_IS_DIR) != 0)
return EISDIR;
- uint32 fileSize = node->dataLen[FS_DATA_FORMAT];
-
- // set/check boundaries for pos/length
- if (pos < 0)
- return B_BAD_VALUE;
- if (pos >= fileSize) {
- *_length = 0;
- return B_OK;
- }
-
return file_cache_read(node->cache, NULL, pos, buffer, _length);
}
diff --git a/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp
b/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp
index 64bc540937..dce9c7613c 100644
--- a/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp
+++ b/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp
@@ -83,12 +83,6 @@ struct PackageFile::DataAccessor {
status_t ReadData(off_t offset, void* buffer, size_t* bufferSize)
{
- if (offset < 0 || (uint64)offset > fData->UncompressedSize())
- return B_BAD_VALUE;
-
- *bufferSize = std::min((uint64)*bufferSize,
- fData->UncompressedSize() - offset);
-
return file_cache_read(fFileCache, NULL, offset, buffer,
bufferSize);
}
diff --git a/src/add-ons/kernel/file_systems/udf/Icb.cpp
b/src/add-ons/kernel/file_systems/udf/Icb.cpp
index f88f227722..4bd780685d 100644
--- a/src/add-ons/kernel/file_systems/udf/Icb.cpp
+++ b/src/add-ons/kernel/file_systems/udf/Icb.cpp
@@ -232,7 +232,7 @@ Icb::FindBlock(uint32 logicalBlock, off_t &block, bool
&recorded)
long_address extent;
bool isEmpty = false;
recorded = false;
-
+
switch (_IcbTag().descriptor_flags()) {
case ICB_DESCRIPTOR_TYPE_SHORT:
{
@@ -299,6 +299,11 @@ Icb::Read(off_t pos, void *buffer, size_t *length, uint32
*block)
TRACE(("Icb::Read: pos = %" B_PRIdOFF ", buffer = %p, length =
(%p)->%ld\n",
pos, buffer, length, (length ? *length : 0)));
+ DEBUG_INIT_ETC("Icb", ("pos: %lld, length: %ld", pos, *length));
+
+ if (fFileCache != NULL)
+ return file_cache_read(fFileCache, NULL, pos, buffer, length);
+
if (!buffer || !length || pos < 0)
return B_BAD_VALUE;
@@ -307,11 +312,6 @@ Icb::Read(off_t pos, void *buffer, size_t *length, uint32
*block)
return B_OK;
}
- DEBUG_INIT_ETC("Icb", ("pos: %lld, length: %ld", pos, *length));
-
- if (fFileCache != NULL)
- return file_cache_read(fFileCache, NULL, pos, buffer, length);
-
switch (_IcbTag().descriptor_flags()) {
case ICB_DESCRIPTOR_TYPE_SHORT:
{
