From f8b944bf7fbbd9745a27bb605159c2ce80d89e27 Mon Sep 17 00:00:00 2001 From: Kyle Bayes Date: Thu, 4 Jul 2024 08:06:34 -0700 Subject: [PATCH] Fix missing fclose when resource is closed without being read. Fixes #1685. PiperOrigin-RevId: 649416747 Change-Id: I05eeb1071868116b6e5be76e26b465788e4d35b1 --- src/engine/engine_resource.c | 25 ++++++++----------------- test/user/user_mesh_test.cc | 14 +++++++------- test/user/user_objects_test.cc | 16 ++++++++-------- 3 files changed, 23 insertions(+), 32 deletions(-) diff --git a/src/engine/engine_resource.c b/src/engine/engine_resource.c index 107bf3b4..1668811c 100644 --- a/src/engine/engine_resource.c +++ b/src/engine/engine_resource.c @@ -14,6 +14,7 @@ #include "engine/engine_resource.h" +#include #include #include #include @@ -41,7 +42,6 @@ static void* _fileToMemory(FILE* fp, const char* filename, size_t* filesize); // file buffer used internally for the OS filesystem typedef struct { - FILE* fp; // file handle int is_read; // set to nonzero if buffer was read into uint8_t* buffer; // raw bytes from file size_t nbuffer; // size of buffer in bytes @@ -100,20 +100,16 @@ mjResource* mju_openResource(const char* name, char* error, size_t error_sz) { spec->is_read = 0; spec->buffer = NULL; spec->nbuffer = 0; - spec->fp = fopen(name, "rb"); - if (!spec->fp) { - if (error) { - snprintf(error, error_sz, - "resource not found via provider or OS filesystem: '%s'", name); - } - mju_closeResource(resource); - return NULL; - } struct stat file_stat; + errno = 0; if (stat(name, &file_stat) == 0) { memcpy(&spec->mtime, &file_stat.st_mtime, sizeof(time_t)); } else { - memset(&spec->mtime, 0, sizeof(time_t)); + if (error) { + snprintf(error, error_sz, "Error opening file '%s': %s", name, strerror(errno)); + } + mju_closeResource(resource); + return NULL; } mju_encodeBase64(resource->timestamp, (uint8_t*) &spec->mtime, sizeof(time_t)); return resource; @@ -157,17 +153,12 @@ int mju_readResource(mjResource* resource, const void** buffer) { return resource->provider->read(resource, buffer); } - // if provider is NULL, then OS filesystem is used file_spec* spec = (file_spec*) resource->data; - if (!spec->fp && !spec->is_read) { - mjERROR("internal error FILE pointer undefined"); // should not occur - } // only read once from file if (!spec->is_read) { - spec->buffer = _fileToMemory(spec->fp, resource->name, &(spec->nbuffer)); - spec->fp = NULL; // closed by _fileToMemory + spec->buffer = mju_fileToMemory(resource->name, &(spec->nbuffer)); spec->is_read = 1; } *buffer = spec->buffer; diff --git a/test/user/user_mesh_test.cc b/test/user/user_mesh_test.cc index 02e6a6c1..8e8d04ff 100644 --- a/test/user/user_mesh_test.cc +++ b/test/user/user_mesh_test.cc @@ -129,7 +129,7 @@ TEST_F(MjCMeshTest, LoadMSHWithVFS) { // should fallback to OS filesystem mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr("resource not found via provider or OS")); + EXPECT_THAT(error, HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -155,7 +155,7 @@ TEST_F(MjCMeshTest, LoadOBJWithVFS) { // should fallback to OS filesystem mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr("resource not found via provider or OS")); + EXPECT_THAT(error, HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -181,7 +181,7 @@ TEST_F(MjCMeshTest, LoadSTLWithVFS) { // should fallback to OS filesystem mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr("resource not found via provider or OS")); + EXPECT_THAT(error, HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -209,7 +209,7 @@ TEST_F(MjCMeshTest, LoadMSHWithContentType) { // should try opening the file (not found obviously) mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr("resource not found via provider or OS")); + EXPECT_THAT(error, HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -235,7 +235,7 @@ TEST_F(MjCMeshTest, LoadOBJWithContentType) { // should try opening the file (not found obviously) mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr("resource not found via provider or OS")); + EXPECT_THAT(error, HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -261,7 +261,7 @@ TEST_F(MjCMeshTest, LoadSTLWithContentType) { // should try opening the file (not found obviously) mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr("resource not found via provider or OS")); + EXPECT_THAT(error, HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -313,7 +313,7 @@ TEST_F(MjCMeshTest, LoadMSHWithContentTypeParam) { // should try opening the file (not found obviously) mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); - EXPECT_THAT(error, HasSubstr("resource not found via provider or OS")); + EXPECT_THAT(error, HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } diff --git a/test/user/user_objects_test.cc b/test/user/user_objects_test.cc index ec58d7fc..f7c90528 100644 --- a/test/user/user_objects_test.cc +++ b/test/user/user_objects_test.cc @@ -69,7 +69,7 @@ TEST_F(VfsTest, HFieldPngWithVFS) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -97,7 +97,7 @@ TEST_F(VfsTest, HFieldCustomWithVFS) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -126,7 +126,7 @@ TEST_F(VfsTest, TexturePngWithVFS) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -155,7 +155,7 @@ TEST_F(VfsTest, TextureCustomWithVFS) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -188,7 +188,7 @@ TEST_F(ContentTypeTest, HFieldPngWithContentType) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -217,7 +217,7 @@ TEST_F(ContentTypeTest, HFieldCustomWithContentType) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -274,7 +274,7 @@ TEST_F(ContentTypeTest, TexturePngWithContentType) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); } @@ -304,7 +304,7 @@ TEST_F(ContentTypeTest, TextureCustomWithContentType) { mjModel* model = LoadModelFromString(xml, error, error_sz, vfs.get()); EXPECT_THAT(model, IsNull()); EXPECT_THAT(error, - HasSubstr("resource not found via provider or OS filesystem")); + HasSubstr("Error opening file")); mj_deleteVFS(vfs.get()); }