[SERVER-31616] Check return value of std::snprintf for errors Created: 18/Oct/17  Updated: 30/Oct/23  Resolved: 03/Nov/17

Status: Closed
Project: Core Server
Component/s: Storage
Affects Version/s: None
Fix Version/s: 3.6.0-rc3

Type: Bug Priority: Major - P3
Reporter: Daniel Gottlieb (Inactive) Assignee: Xiangyu Yao (Inactive)
Resolution: Fixed Votes: 0
Labels: neweng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Storage 2017-11-13
Participants:
Linked BF Score: 0

 Description   

There are four cases of using std::snprintf in db/storage that check the return code for some errors (a partial write), but not for the -1 error value. E.g:

    auto size = std::snprintf(oldestTSConfigString,
                              sizeof(oldestTSConfigString),
                              "oldest_timestamp=%llx",
                              static_cast<unsigned long long>(timestampToSet.asU64()));
    invariant(static_cast<std::size_t>(size) < sizeof(oldestTSConfigString));

This work should include checking the return value for -1 and translating the errno.

Grep:
https://github.com/mongodb/mongo/blob/0b23e5ab80c96a8c5b001a1a33c5d6099c48ef2b/src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp#L1045-L1049
https://github.com/mongodb/mongo/blob/0b23e5ab80c96a8c5b001a1a33c5d6099c48ef2b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp#L1669-L1674
https://github.com/mongodb/mongo/blob/0b23e5ab80c96a8c5b001a1a33c5d6099c48ef2b/src/mongo/db/storage/wiredtiger/wiredtiger_snapshot_manager.cpp#L85-L89
https://github.com/mongodb/mongo/blob/0b23e5ab80c96a8c5b001a1a33c5d6099c48ef2b/src/mongo/db/storage/wiredtiger/wiredtiger_snapshot_manager.cpp#L112-L116



 Comments   
Comment by Githook User [ 03/Nov/17 ]

Author:

{'name': 'Xiangyu Yao', 'username': 'xy24', 'email': 'xiangyu.yao@mongodb.com'}

Message: SERVER-31616 Check return value of std::snprintf for errors
Branch: master
https://github.com/mongodb/mongo/commit/33fc9ea009d3d91fde291fc19a9770510f8b189b

Comment by Eric Milkie [ 02/Nov/17 ]

std::snprintf should never fail (return -1) so I think we should just crash. Be sure to output the translated errno (errnoWithDescription()) first.

Comment by Daniel Gottlieb (Inactive) [ 02/Nov/17 ]

I think the correct behavior is different for each case:

  1. If the method returns a Status, consider returning an error and the caller can retry at their own leisure. I think that's appropriate for setting the read timestamp on oplog transactions or otherwise
  2. Retry a handful of times until it succeeds (would sleeping between tries help? I have no idea). If snprintf continues to fail, invarianting may be our only choice. This unfortunately may be necessary for rewinding visibility on rollback.
  3. Just return without calling the WT method that wants a timestamp. This may be appropriate for updating the oldest_timestamp. Missing a few updates here and there is not problematic. Not updating the oldest_timestamp for a long time would be asking for trouble. I think trying to catch that case would be out of scope for this ticket.

Thoughts milkie?

Comment by Xiangyu Yao (Inactive) [ 02/Nov/17 ]

daniel.gottlieb Hey Dan, do we want to throw an exception(invariant) or output the error + skip _conn.set_timestamp() when catching the error?

Generated at Thu Feb 08 04:27:39 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.