[CDRIVER-3811] mongoc_gridfs_create_file_from_stream() has inconsistent error handling semantics Created: 29/Oct/20  Updated: 03/Jun/21  Resolved: 03/Jun/21

Status: Closed
Project: C Driver
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Michael Mlsna Assignee: Unassigned
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

Although the documentation for mongoc_gridfs_create_file_from_stream() does not clearly state whether the user is responsible for cleaning up the mongoc_stream_t on success/failure, from reading the source code, examples, and tests in the repo, it seems like the intended contract is:

  • on success, the mongoc_stream_t will be cleaned up by the callee
  • on failure, the mongoc_stream_t should be cleaned up by the caller

However, callers cannot simply call mongoc_stream_destroy() if the function returns NULL, because the stream is only valid on certain error paths, not all error paths.

Looking at https://github.com/mongodb/mongo-c-driver/blob/1.17.1/src/libmongoc/src/mongoc/mongoc-stream.c#L78:

   for (;;) {
      r = mongoc_stream_read (
         stream, iov.iov_base, MONGOC_GRIDFS_STREAM_CHUNK, 0, timeout);
 
      if (r > 0) {
         iov.iov_len = r;
         if (mongoc_gridfs_file_writev (file, &iov, 1, timeout) < 0) {
            MONGOC_ERROR ("%s", file->error.message);
            mongoc_gridfs_file_destroy (file);
            RETURN (NULL); // #1
         }
      } else if (r == 0) {
         break;
      } else {
         MONGOC_ERROR ("Error reading from GridFS file source stream");
         mongoc_gridfs_file_destroy (file);
         RETURN (NULL); // #2
      }
   }
 
   mongoc_stream_failed (stream); // <--- stream destroyed here
 
   if (-1 == mongoc_gridfs_file_seek (file, 0, SEEK_SET)) {
      MONGOC_ERROR ("%s", file->error.message);
      mongoc_gridfs_file_destroy (file);
      RETURN (NULL); // #3
   }

The three NULL return paths have been labeled. For #1 and #2, the mongoc_stream_t will be valid after the call returns, but for #3 it will have already been deallocated by mongoc_stream_failed().

Given there is no way to programmatically distinguish the error return paths, the only safe behavior currently is to avoid calling mongoc_stream_destroy() on error, which leads to resource leaks.



 Comments   
Comment by Rachelle Palmer [ 03/Jun/21 ]

We've decided not to fix this issue as it is for our deprecated API.

Comment by Michael Mlsna [ 03/Nov/20 ]

I appreciate the quick turnaround for triaging this. And thank you for pointing out the mongoc_gridfs_bucket_t API, somehow I completely missed the big deprecation warning on the API Reference site . I'll take a look at upgrading to the new GridFS API.

Comment by Kevin Albertson [ 03/Nov/20 ]

Hi mmlsna@vailsys.com, thank you for the detailed report! I agree this appears to be a bug.

At a glance, I am thinking of these solutions to make it unambiguous:

Solution A:
Remove the call to mongoc_stream_failed(). The caller must always destroy the stream, despite the return.

Solution B:
Call mongoc_stream_failed() in case #1, and mongoc_stream_destroy() in case #2. The caller must NOT destroy the stream when NULL is returned.

Solution A seems more appealing. It seems like a less harmful backwards break of behavior than solution B.

We will fix this as soon as we can, though we also welcome pull requests against the project if you are interested in contributing.

Also note, mongoc_gridfs_t and the associated API has been superseded by the mongoc_gridfs_bucket_t API (documented here), which adheres to the cross-driver GridFS specification. If possible, we recommend switching to that API.

Generated at Wed Feb 07 21:19:06 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.