From 5f745f407433302b3942dd40fa23fcda70d04594 Mon Sep 17 00:00:00 2001 From: uramer Date: Sat, 21 Dec 2024 16:43:21 +0100 Subject: [PATCH 1/3] Reference the FileHandle Lua object from lines closure tro prevent garbage collection --- apps/openmw/mwlua/vfsbindings.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/apps/openmw/mwlua/vfsbindings.cpp b/apps/openmw/mwlua/vfsbindings.cpp index 3186db26ca..e49a8718f5 100644 --- a/apps/openmw/mwlua/vfsbindings.cpp +++ b/apps/openmw/mwlua/vfsbindings.cpp @@ -189,10 +189,11 @@ namespace MWLua return seek(lua, self, std::ios_base::cur, off); }); - handle["lines"] = [](sol::this_state lua, FileHandle& self) { - return sol::as_function([&lua, &self]() mutable { - validateFile(self); - return readLineFromFile(lua, self); + handle["lines"] = [](sol::this_state lua, sol::object self) { + return sol::as_function([lua, self]() { + FileHandle* handle = self.as(); + validateFile(*handle); + return readLineFromFile(lua, *handle); }); }; From 6d8753e5a853a4d5a3aa01dea8407d6af8365d82 Mon Sep 17 00:00:00 2001 From: uramer Date: Sat, 21 Dec 2024 16:47:19 +0100 Subject: [PATCH 2/3] Fix crash if someone evil calls the .lines method on a non-file --- apps/openmw/mwlua/vfsbindings.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/openmw/mwlua/vfsbindings.cpp b/apps/openmw/mwlua/vfsbindings.cpp index e49a8718f5..4a71a29581 100644 --- a/apps/openmw/mwlua/vfsbindings.cpp +++ b/apps/openmw/mwlua/vfsbindings.cpp @@ -190,7 +190,9 @@ namespace MWLua return seek(lua, self, std::ios_base::cur, off); }); handle["lines"] = [](sol::this_state lua, sol::object self) { - return sol::as_function([lua, self]() { + return sol::as_function([lua, self]() -> sol::object { + if (!self.is()) + return sol::nil; FileHandle* handle = self.as(); validateFile(*handle); return readLineFromFile(lua, *handle); From c9ffd978ec449ed153e81ac44e3f3b108e1da85f Mon Sep 17 00:00:00 2001 From: Evil Eye Date: Thu, 2 Jan 2025 15:55:19 +0100 Subject: [PATCH 3/3] Hoist the FileHandle check and add more tests --- apps/openmw/mwlua/vfsbindings.cpp | 4 ++-- .../data/integration_tests/test_lua_api/test.lua | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/apps/openmw/mwlua/vfsbindings.cpp b/apps/openmw/mwlua/vfsbindings.cpp index 4a71a29581..cf1fa02815 100644 --- a/apps/openmw/mwlua/vfsbindings.cpp +++ b/apps/openmw/mwlua/vfsbindings.cpp @@ -190,9 +190,9 @@ namespace MWLua return seek(lua, self, std::ios_base::cur, off); }); handle["lines"] = [](sol::this_state lua, sol::object self) { + if (!self.is()) + throw std::runtime_error("self should be a file handle"); return sol::as_function([lua, self]() -> sol::object { - if (!self.is()) - return sol::nil; FileHandle* handle = self.as(); validateFile(*handle); return readLineFromFile(lua, *handle); diff --git a/scripts/data/integration_tests/test_lua_api/test.lua b/scripts/data/integration_tests/test_lua_api/test.lua index 78f2d956f7..ff2cd9bb33 100644 --- a/scripts/data/integration_tests/test_lua_api/test.lua +++ b/scripts/data/integration_tests/test_lua_api/test.lua @@ -227,16 +227,18 @@ end local function testVFS() local file = 'test_vfs_dir/lines.txt' + local nosuchfile = 'test_vfs_dir/nosuchfile' testing.expectEqual(vfs.fileExists(file), true, 'lines.txt should exist') - testing.expectEqual(vfs.fileExists('test_vfs_dir/nosuchfile'), false, 'nosuchfile should not exist') + testing.expectEqual(vfs.fileExists(nosuchfile), false, 'nosuchfile should not exist') + local expectedLines = { '1', '2', '', '4' } local getLine = vfs.lines(file) - for _,v in pairs({ '1', '2', '', '4' }) do + for _,v in pairs(expectedLines) do testing.expectEqual(getLine(), v) end testing.expectEqual(getLine(), nil, 'All lines should have been read') local ok = pcall(function() - vfs.lines('test_vfs_dir/nosuchfile') + vfs.lines(nosuchfile) end) testing.expectEqual(ok, false, 'Should not be able to read lines from nonexistent file') @@ -259,6 +261,14 @@ local function testVFS() testing.expectEqual(handle:close(), true, 'File should be closeable') testing.expectEqual(vfs.type(handle), 'closed file', 'File should be closed') + + handle = vfs.open(nosuchfile) + testing.expectEqual(handle, nil, 'vfs.open should return nil on nonexistent files') + + getLine = vfs.open(file):lines() + for _,v in pairs(expectedLines) do + testing.expectEqual(getLine(), v) + end end local function testCommitCrime()