Update fdbserver to use fmt library consistently

[tclinkenbeard-oai]

This PR applies a similar fmt rewrite tool as used in #12780, this time for fdbserver.

While applying this rewrite, 2 bugs with invalid print statements were fixed in DataDistribution.actor.cpp (in the second commit of this PR).

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

• The PR has a description, explaining both the problem and the solution.
• The description mentions which forms of testing were done and the testing seems reasonable.
• Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

• This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
• There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: 988dfdb
• Duration 0:04:21
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-ide on Linux RHEL 9

• Commit ID: 988dfdb
• Duration 0:04:21
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: 988dfdb
• Duration 0:04:24
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

• Commit ID: 988dfdb
• Duration 0:04:26
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-arm on Linux CentOS 7

• Commit ID: 988dfdb
• Duration 0:04:27
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

• Commit ID: 988dfdb
• Duration 0:20:26
• Result: ❌ FAILED
• Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /opt/homebrew/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos on macOS Ventura 13.x

• Commit ID: 988dfdb
• Duration 0:25:04
• Result: ❌ FAILED
• Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-ide on Linux RHEL 9

• Commit ID: 242f54f
• Duration 0:04:24
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: 242f54f
• Duration 0:04:21
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: 242f54f
• Duration 0:04:21
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

• Commit ID: 242f54f
• Duration 0:04:24
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-arm on Linux CentOS 7

• Commit ID: 242f54f
• Duration 0:04:31
• Result: ❌ FAILED
• Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

• Commit ID: 242f54f
• Duration 0:35:11
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos on macOS Ventura 13.x

• Commit ID: 242f54f
• Duration 0:52:02
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-ide on Linux RHEL 9

• Commit ID: 7a6f0d7
• Duration 0:21:05
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-arm on Linux CentOS 7

• Commit ID: 7a6f0d7
• Duration 0:40:39
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: 7a6f0d7
• Duration 0:54:56
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: 7a6f0d7
• Duration 0:56:13
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[gxglass]

I personally don't find the {} stuff easier to read. I don't care about visual compatibility with python. Just tell me what the type is.

If we need to call format to print a complex data structure that's another question and I guess we can do that. But in general I'd rather read %d than {} in a format string

[gxglass]

Also I looked at fmt.dev and it says "Errors in format strings, which are a common source of vulnerabilities in C, are reported at compile time. For example ... ".

Pretty sure C and C++ compilers have been warning about this for about 25 years. If we can point to concrete examples where format string bugs have got through I would be interested to learn about it.

[gxglass]

Also I looked at fmt.dev and it says "Errors in format strings, which are a common source of vulnerabilities in C, are reported at compile time. For example ... ".

Pretty sure C and C++ compilers have been warning about this for about 25 years. If we can point to concrete examples where format string bugs have got through I would be interested to learn about it.

I checked the bugs in the second commit. Those are existing calls to format where the wrong object is passed. Presumably something gets printed but we emit a confusing message. What I'm wondering about is actual vulnerabilities due to use of type-specific format strings.

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

• Commit ID: 7a6f0d7
• Duration 2:04:38
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)

[gxglass]

I personally don't find the {} stuff easier to read. I don't care about visual compatibility with python. Just tell me what the type is.

If we need to call format to print a complex data structure that's another question and I guess we can do that. But in general I'd rather read %d than {} in a format string

I don't feel super strongly about the above and would like the team to weigh in.

[tclinkenbeard-oai]

If we can point to concrete examples where format string bugs have got through I would be interested to learn about it.

I can't find any concrete examples in this PR, but one source of potential undefined behavior is

foundationdb/fdbrpc/HTTP.actor.cpp

Line 50 in d8fc979

sprintf(buf, "%%%.02X", c);

, where if a URI has non-ASCII characters, c will be negative, and the sprintf call is undefined on some platforms.

[tclinkenbeard-oai]

I personally don't find the {} stuff easier to read. I don't care about visual compatibility with python. Just tell me what the type is.

If we need to call format to print a complex data structure that's another question and I guess we can do that. But in general I'd rather read %d than {} in a format string

One possibility is that the fmt function calls can be updated to show type information, which is statically checked, e.g. {:d} instead of {}

[gxglass]

If we can point to concrete examples where format string bugs have got through I would be interested to learn about it.

I can't find any concrete examples in this PR, but one source of potential undefined behavior is

foundationdb/fdbrpc/HTTP.actor.cpp

Line 50 in d8fc979

sprintf(buf, "%%%.02X", c);

, where if a URI has non-ASCII characters, c will be negative, and the sprintf call is undefined on some platforms.

Ah yeah, somebody sent a security note about this. My view on this is that the original code is just unjustifiably wrong. Nobody should be typing sprintf in the 21st century. It should have been snprintf from the beginning. So basically, a simple solution is both sufficient and necessary here and it doesn't necessarily warrant fmt.

[gxglass]

If we can point to concrete examples where format string bugs have got through I would be interested to learn about it.

I can't find any concrete examples in this PR, but one source of potential undefined behavior is

foundationdb/fdbrpc/HTTP.actor.cpp

Line 50 in d8fc979

sprintf(buf, "%%%.02X", c);

, where if a URI has non-ASCII characters, c will be negative, and the sprintf call is undefined on some platforms.

Ah yeah, somebody sent a security note about this. My view on this is that the original code is just unjustifiably wrong. Nobody should be typing sprintf in the 21st century. It should have been snprintf from the beginning. So basically, a simple solution is both sufficient and necessary here and it doesn't necessarily warrant fmt.

I'll address the sprintf in a separate PR. We keep getting security reports about this anyway.

[gxglass]

@spraza @saintstack Do you have any views on the fmt usage here?

[spraza]

@spraza @saintstack Do you have any views on the fmt usage here?

I do prefer explicit types. Also the DataDistributor commit is probably fixing a copy-paste bug, doesn't seem like it warrants fmt.

On the plus side, maybe potential perf win, although we haven't measured. I do like the ability to reuse positional arguments. And finally, printing complex data structures should be easier.

Overall I'm ok with the change, I think it's a net positive with lack of explicit types being the main downside, which we can address (e.g. {:d}).