[SERVER-47203] LOGV2 macros should protect against forgetting trailing commas to separate arguments Created: 31/Mar/20  Updated: 29/Oct/23  Resolved: 04/Oct/23

Status: Closed
Project: Core Server
Component/s: Logging
Affects Version/s: None
Fix Version/s: 7.2.0-rc0

Type: Improvement Priority: Major - P3
Reporter: Max Hirschhorn Assignee: Backlog - Security Team
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-81602 Remove support for formatMsg from LOGV2 Closed
Related
is related to SERVER-46799 Update sharding log lines to adhere t... Closed
Assigned Teams:
Server Security
Backwards Compatibility: Fully Compatible
Participants:

 Description   

One of the most common mistakes observed during the code reviews for updating the sharding log lines based on the LOGV2 style guide (see SERVER-46799) was omitting a trailing comma to separate the format string from the message string, or to separate the message string from the first attribute name. There were 30 instances of this mistake made across the 10 individuals (myself included) who worked on those changes.

diff --git a/src/mongo/s/server.cpp b/src/mongo/s/server.cpp
index 960d189008..04a7937b2d 100644
--- a/src/mongo/s/server.cpp
+++ b/src/mongo/s/server.cpp
@@ -651,7 +651,7 @@ ExitCode runMongosServer(ServiceContext* serviceContext) {
     if (!status.isOK()) {
         LOGV2_ERROR(22858,
                     "Error initializing authorization data: {error}",
-                    "Error initializing authorization data"
+                    "Error initializing authorization data",
                     "error"_attr = status);
         return EXIT_SHARDING_ERROR;
     }

This likely occurred due to the way in which engineers went about copy-pasting from the format string to create a sensible message string. It'd be great if the LOGV2 macros could protect against this, although perhaps unnecessary if we aren't going to require format strings in addition to message strings for new log messages. My idea for an approach would be to have the format and message strings be distinct nominal types and to have user-defined suffixes for creating the appropriate type. The compiler would error if two string literals with different ud-suffixes were attempted to be concatenated.



 Comments   
Comment by Billy Donahue [ 04/Oct/23 ]

fixed by SERVER-81602, after which there are no more fmt-message strings.

Comment by Lauren Lewis (Inactive) [ 24/Feb/22 ]

We haven’t heard back from you for at least one calendar year, so this issue is being closed. If this is still an issue for you, please provide additional information and we will reopen the ticket.

Comment by Max Hirschhorn [ 31/Mar/20 ]

In order to catch these mistakes during code review, I would apply the patches locally and then spot-check all matches of /"$/ in the diff.

Generated at Thu Feb 08 05:13:34 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.