Uploaded image for project: 'C Driver'
  1. C Driver
  2. CDRIVER-3811

mongoc_gridfs_create_file_from_stream() has inconsistent error handling semantics

    XMLWordPrintableJSON

Details

    • Icon: Bug Bug
    • Resolution: Won't Fix
    • Icon: Major - P3 Major - P3
    • None
    • None
    • None
    • None

    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.

      Attachments

        Activity

          People

            Unassigned Unassigned
            mmlsna@vailsys.com Michael Mlsna
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: