[CDRIVER-1903] Bugs in _mongoc_gridfs_file_extend Created: 03/Nov/16  Updated: 14/Nov/16  Resolved: 14/Nov/16

Status: Closed
Project: C Driver
Component/s: GridFS, libmongoc
Affects Version/s: 1.3.0
Fix Version/s: 1.5.0

Type: Bug Priority: Major - P3
Reporter: A. Jesse Jiryu Davis Assignee: A. Jesse Jiryu Davis
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to CDRIVER-1896 Coverity analysis defect 76624: Side ... Closed

 Description   

_mongoc_gridfs_file_extend has at least two clearly incorrect statements. First:

      /* Set bytes until we reach the limit or fill a page */
      file->pos += _mongoc_gridfs_file_page_memset0 (file->page,
                                                     target_length - file->pos);

This is wrong because the return type of _mongoc_gridfs_file_page_memset0 is bool, and it is being added to a uint64_t. (Introduced in e2c174.) Second:

   BSON_ASSERT (file->length = target_length);

This appears wrong because it is an assignment in an assert (CDRIVER-1896), but it is not compiled out in a release build any more (CDRIVER-697). The intention is mysterious.



 Comments   
Comment by Githook User [ 14/Nov/16 ]

Author:

{u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}

Message: CDRIVER-1903 truly fix gridfs write test in MSVC

I had not known that _vsnprintf_s in debug mode fills the buffer out to
"len" with 0xFE.
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/57899f0ac172bffaef77720cb7bd5884d60babc6

Comment by A. Jesse Jiryu Davis [ 12/Nov/16 ]

My new test is failing on Windows.

Comment by Githook User [ 12/Nov/16 ]

Author:

{u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}

Message: CDRIVER-1903 disable gridfs write test in MSVC

Until I can diagnose the issue.
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/883cc25eae75f9bea67d24ad4689a9a70103a04c

Comment by Githook User [ 12/Nov/16 ]

Author:

{u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}

Message: CDRIVER-1903 fix gridfs-write test in MSVC
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/bcc962e91f4414b6a3f24d2bdb6d08ad5d7fab29

Comment by Githook User [ 11/Nov/16 ]

Author:

{u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}

Message: CDRIVER-1903 fix gridfs-write test
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/72351e9926001a03b51143648746598ec53163cb

Comment by Githook User [ 08/Nov/16 ]

Author:

{u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}

Message: CDRIVER-1903 bugs in _mongoc_gridfs_file_extend
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/5aecadcaf701e64c6a3aaae09255d36dbf50229d

Comment by A. Jesse Jiryu Davis [ 06/Nov/16 ]

The file->pos += bool statement is a quadratic performance bug: it zeroes the bytes from "pos" to "target_length", advances "pos" by one byte, and zeroes the bytes from "pos" to "target_length" again, until "pos" equals "target_length". The solution is to advance "pos" by the actual number of bytes that were zeroed.

The assignment in the assert is just a wart: we do want to set "length" to "target_length", and since the statement is always executed, even in a release build, the logic is correct. The assert never fires since _mongoc_gridfs_file_extend is never called with a zero-length file. The solution is to remove the assert and leave the assignment in place.

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