[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: |
|
||||||||
| Description |
|
_mongoc_gridfs_file_extend has at least two clearly incorrect statements. First:
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:
This appears wrong because it is an assignment in an assert ( |
| 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: I had not known that _vsnprintf_s in debug mode fills the buffer out to |
| 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: Until I can diagnose the issue. |
| 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: |
| 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: |
| 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: |
| 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. |