1673917 - Overloading can cause unexpected version of nsTSubString::AppendPrintf to be selected

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Based on behaviour seen in bug 1673670 MSVC has some unexpected behaviour when dealing with overloads where both a printf and a vprintf style function are available (a reduced case can be seen at https://godbolt.org/z/MasGv4). Even though we use clang on Windows, we still use some parts of Microsoft's tooling which makes this an issue.

This means that nsTSubString::AppendPrintf[0] may be resolved unexpectedly when using a char array cast to a pointer (edit: or any non const char*). It's not clear to me if there are other cases that could trigger this, but it is desirable to avoid the footgun this presents.

The vprintf version of AppendPrintf is used at a relatively small number of call sites, so I think renaming the function to avoid overloading should be fairly painless. I'll look into doing so + getting some gtest coverage.

[0] https://searchfox.org/mozilla-central/rev/c409dd9235c133ab41eba635f906aa16e050c197/xpcom/string/nsTSubstring.cpp#1203,1215

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1673917 - Add gtest for nsTSubString::AppendPrintf. r?sg

This adds some coverage for nsTSubString::AppendPrintf. These tests will be
expanded in future patches, but this gives some baseline coverage. Importantly
for the bug at hand, this provides a test case that shows the unexpected
overload resolution on Windows.

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1673917 - Rename vprintf style nsTSubstring::AppendPrintf -> AppendVprintf. r?sg

This patch also expands tests to check that the results of creating string via
both methods are consistent.

Depends on D95051

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Try push showing new tests providing coverage and getting weird on Windows https://treeherder.mozilla.org/#/jobs?repo=try&revision=f53d1c1a4ba56510ec8866d547e62d655e81689b&selectedTaskRun=dUsk3RSaTyirIuoo4UmjTQ.0

Comment 4

[Simon Giesecke [:sg] [he/him]]

Other cases that might trigger this might only be on error/exceptional code paths, which are not covered by tests.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cafb95a0288b
Add gtest for nsTSubString::AppendPrintf. r=sg
https://hg.mozilla.org/integration/autoland/rev/eeb7837a1c06
Rename vprintf style nsTSubstring::AppendPrintf -> AppendVprintf. r=sg

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cafb95a0288b
https://hg.mozilla.org/mozilla-central/rev/eeb7837a1c06