Fix missing fclose when resource is closed without being read. Fixes #1685.

PiperOrigin-RevId: 649416747
Change-Id: I05eeb1071868116b6e5be76e26b465788e4d35b1

src/engine/engine_resource.c

@@ -14,6 +14,7 @@
 
 #include "engine/engine_resource.h"
 
+#include <errno.h>
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
@@ -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->buffer = mju_fileToMemory(resource->name, &(spec->nbuffer));
-    spec->fp = NULL;  // closed by _fileToMemory
     spec->is_read = 1;
   }
   *buffer = spec->buffer;

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());
 }
 

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());
 }