Skip to content

Fix incorrect format string arguments #67464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 12, 2022
Merged

Fix incorrect format string arguments #67464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ba172e
Fix cpp/wrong-type-format-argument
GrabYourPitchforks Mar 28, 2022
b6c103e
Remove <cinttypes> from src/coreclr; update zd -> zu
GrabYourPitchforks Apr 5, 2022
3871167
Remove %zx format strings from coreclr tree
GrabYourPitchforks Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/coreclr/tools/metainfo/mdinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ void MDInfo::DisplayMD()
// WriteLine("Unresolved MemberRefs");
// DisplayMemberRefs(0x00000001, "\t");

VWrite("\n\nCoff symbol name overhead: %d\n", g_cbCoffNames);
VWrite("\n\nCoff symbol name overhead: %Iu\n", g_cbCoffNames);
}
WriteLine("===========================================================");
if (m_DumpFilter & dumpUnsat)
Expand Down Expand Up @@ -1939,7 +1939,7 @@ void MDInfo::DisplayCustomAttributeInfo(mdCustomAttribute inValue, const char *p

VWrite("%s\tCustomAttributeName: %ls", preFix, rcName);
if (pSig && pMethName)
VWrite(" :: %S", qSigName.Ptr());
VWrite(" :: %S", (LPWSTR)qSigName.Ptr());

// Keep track of coff overhead.
if (!wcscmp(W("__DecoratedName"), rcName))
Expand Down Expand Up @@ -2039,7 +2039,7 @@ void MDInfo::DisplayCustomAttributeInfo(mdCustomAttribute inValue, const char *p
case ELEMENT_TYPE_U8:
CA.GetU8(&u8);
uI64 = u8;
VWrite("%#lx", uI64);
VWrite("%#I64x", uI64);
break;
case ELEMENT_TYPE_R4:
dblVal = CA.GetR4();
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/clrex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,7 +1138,7 @@ void EEResourceException::GetMessage(SString &result)
// since we don't want to call managed code here.
//

result.Printf("%s (message resource %s)",
result.Printf("%s (message resource %S)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result.Printf("%s (message resource %S)",
result.Printf(W("%S (message resource %s)"),

This may fix the test failure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Was using this as a good excuse to learn how to use runfo. 😊

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stack trace shows that the failure is this line:

- strArgument.Printf(W("0x%x"), pInstruction->uArg);
+ strArgument.Printf(W("0x%zx"), pInstruction->uArg);

But since ILInstruction.uArg is typed as UINT_PTR, it seems like the new line should be correct?

I'm pretty sure that in context, this value always fits into 32 bits anyway, so maybe the answer is simply to cast back to uint32 and leave the original format string as-is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert says that _vsnwprintf_s failed with unexpected error. Notice that all failures with this error are on non-Windows. I think it means that _vsnwprintf_s implementation in the Unix PAL is missing handling of zx format string.

Copy link
Member Author

@GrabYourPitchforks GrabYourPitchforks Apr 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be ok for me to tweak https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/safecrt/output.inl to recognize this pattern as part of this PR? I could also just use I since that's recognized, but it might make the call site easier to understand if I support using the standards-compliant z.

Edit: Looks like we'd need some more generalized cleanup throughout this if we wanted to support standard syntaxes. I'll just use I for now since it doesn't add much debt to what's already there.

CoreLibBinder::GetExceptionName(m_kind), m_resourceName.GetUnicode());
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/eepolicy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ void EEPolicy::LogFatalError(UINT exitCode, UINT_PTR address, LPCWSTR pszMessage
if(!ssErrorFormat.LoadResource(CCompRC::Optional, IDS_ER_UNMANAGEDFAILFASTMSG ))
ssErrorFormat.Set(W("at IP 0x%x (0x%x) with exit code 0x%x."));
SmallStackSString addressString;
addressString.Printf(W("%p"), pExceptionInfo? (UINT_PTR)pExceptionInfo->ExceptionRecord->ExceptionAddress : address);
addressString.Printf(W("%p"), pExceptionInfo? (PVOID)pExceptionInfo->ExceptionRecord->ExceptionAddress : (PVOID)address);

// We should always have the reference to the runtime's instance
_ASSERTE(GetClrModuleBase() != NULL);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/eventreporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ void DoReportForUnhandledNativeException(PEXCEPTION_POINTERS pExceptionInfo)
SmallStackSString exceptionCodeString;
exceptionCodeString.Printf(W("%x"), pExceptionInfo->ExceptionRecord->ExceptionCode);
SmallStackSString addressString;
addressString.Printf(W("%p"), (UINT_PTR)pExceptionInfo->ExceptionRecord->ExceptionAddress);
addressString.Printf(W("%p"), (PVOID)pExceptionInfo->ExceptionRecord->ExceptionAddress);
s.FormatMessage(FORMAT_MESSAGE_FROM_STRING, (LPCWSTR)ssErrorFormat, 0, 0, exceptionCodeString, addressString);
reporter.AddDescription(s);
if (pThread)
Expand Down
12 changes: 6 additions & 6 deletions src/coreclr/vm/stubgen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ ILStubLinker::LogILInstruction(

if (isLabeled)
{
strLabel.Printf(W("IL_%04x:"), curOffset);
strLabel.Printf(W("IL_%04Ix:"), curOffset);
}
else
{
Expand Down Expand Up @@ -502,7 +502,7 @@ ILStubLinker::LogILInstruction(
{
size_t branchDistance = (size_t)pInstruction->uArg;
size_t targetOffset = curOffset + s_rgbOpcodeSizes[instr] + branchDistance;
strArgument.Printf(W("IL_%04x"), targetOffset);
strArgument.Printf(W("IL_%04Ix"), targetOffset);
}
else if ((ILCodeStream::ILInstrEnum)CEE_NOP == instr)
{
Expand All @@ -520,7 +520,7 @@ ILStubLinker::LogILInstruction(
case ShortInlineVar:
case ShortInlineI:
case InlineI:
strArgument.Printf(W("0x%x"), pInstruction->uArg);
strArgument.Printf(W("0x%Ix"), pInstruction->uArg);
break;

case InlineI8:
Expand All @@ -536,7 +536,7 @@ ILStubLinker::LogILInstruction(
case InlineTok:
// No token value when we dump IL for ETW
if (pDumpILStubCode == NULL)
strArgument.Printf(W("0x%08x"), pInstruction->uArg);
strArgument.Printf(W("0x%08Ix"), pInstruction->uArg);

// Dump to szTokenNameBuffer if logging, otherwise dump to szArgumentBuffer to avoid an extra space because we are omitting the token
_ASSERTE(FitsIn<mdToken>(pInstruction->uArg));
Expand Down Expand Up @@ -620,11 +620,11 @@ ILStubLinker::LogILStubWorker(
{
if (pDumpILStubCode)
{
pDumpILStubCode->AppendPrintf(W("IL_%04x:\n"), *pcbCode);
pDumpILStubCode->AppendPrintf(W("IL_%04Ix:\n"), *pcbCode);
}
else
{
LOG((LF_STUBS, LL_INFO1000, "IL_%04x:\n", *pcbCode));
LOG((LF_STUBS, LL_INFO1000, "IL_%04Ix:\n", *pcbCode));
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/native/corehost/bundle/extractor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "dir_utils.h"
#include "pal.h"
#include "utils.h"
#include <cinttypes>

#ifdef __sun
#include <alloca.h>
Expand Down Expand Up @@ -178,7 +179,7 @@ void extractor_t::extract(const file_entry_t &entry, reader_t &reader)

if (extracted_size != cast_size)
{
trace::error(_X("Failure extracting contents of the application bundle. Expected size:%d Actual size:%d"), size, extracted_size);
trace::error(_X("Failure extracting contents of the application bundle. Expected size:%" PRId64 " Actual size:%zu"), size, extracted_size);
trace::error(_X("I/O failure when writing extracted files."));
throw StatusCode::BundleExtractionIOError;
}
Expand Down
5 changes: 3 additions & 2 deletions src/native/corehost/bundle/info.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "trace.h"
#include "info.h"
#include "utils.h"
#include <cinttypes>

using namespace bundle;

Expand Down Expand Up @@ -48,8 +49,8 @@ StatusCode info_t::process_bundle(const pal::char_t* bundle_path, const pal::cha
}

trace::info(_X("Single-File bundle details:"));
trace::info(_X("DepsJson Offset:[%lx] Size[%lx]"), info.m_header.deps_json_location().offset, info.m_header.deps_json_location().size);
trace::info(_X("RuntimeConfigJson Offset:[%lx] Size[%lx]"), info.m_header.runtimeconfig_json_location().offset, info.m_header.runtimeconfig_json_location().size);
trace::info(_X("DepsJson Offset:[%" PRIx64 "] Size[%" PRIx64 "]"), info.m_header.deps_json_location().offset, info.m_header.deps_json_location().size);
trace::info(_X("RuntimeConfigJson Offset:[%" PRIx64 "] Size[%" PRIx64 "]"), info.m_header.runtimeconfig_json_location().offset, info.m_header.runtimeconfig_json_location().size);
trace::info(_X(".net core 3 compatibility mode: [%s]"), info.m_header.is_netcoreapp3_compat_mode() ? _X("Yes") : _X("No"));

the_app = &info;
Expand Down
3 changes: 2 additions & 1 deletion src/native/corehost/corehost.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "trace.h"
#include "utils.h"
#include "hostfxr_resolver.h"
#include <cinttypes>

#if defined(FEATURE_APPHOST)
#include "bundle_marker.h"
Expand Down Expand Up @@ -200,7 +201,7 @@ int exe_start(const int argc, const pal::char_t* argv[])
trace::info(_X("Host path: [%s]"), host_path.c_str());
trace::info(_X("Dotnet path: [%s]"), fxr.dotnet_root().c_str());
trace::info(_X("App path: [%s]"), app_path.c_str());
trace::info(_X("Bundle Header Offset: [%lx]"), bundle_header_offset);
trace::info(_X("Bundle Header Offset: [%" PRId64 "]"), bundle_header_offset);

auto set_error_writer = fxr.resolve_set_error_writer();
propagate_error_writer_t propagate_error_writer_to_hostfxr(set_error_writer);
Expand Down
2 changes: 1 addition & 1 deletion src/native/corehost/deps_format.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ void deps_json_t::reconcile_libraries_with_targets(

if (trace::is_enabled())
{
trace::info(_X("Parsed %s deps entry %d for asset name: %s from %s: %s, library version: %s, relpath: %s, assemblyVersion %s, fileVersion %s"),
trace::info(_X("Parsed %s deps entry %zu for asset name: %s from %s: %s, library version: %s, relpath: %s, assemblyVersion %s, fileVersion %s"),
deps_entry_t::s_known_asset_types[i],
m_deps_entries[i].size() - 1,
entry.asset.name.c_str(),
Expand Down
4 changes: 2 additions & 2 deletions src/native/corehost/fxr/command_line.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ namespace
for (const auto& opt : known_opts)
{
const host_option &arg = get_host_option(opt);
trace::error(_X(" %s %-*s %s"), arg.option, 36 - pal::strlen(arg.option), arg.argument, arg.description);
trace::error(_X(" %s %-*s %s"), arg.option, 36 - (int)pal::strlen(arg.option), arg.argument, arg.description);
}
return StatusCode::InvalidArgFailure;
}
Expand Down Expand Up @@ -329,7 +329,7 @@ void command_line::print_muxer_usage(bool is_sdk_present)
for (const auto& opt : known_opts)
{
const host_option &arg = get_host_option(opt);
trace::println(_X(" %s %-*s %s"), arg.option, 29 - pal::strlen(arg.option), arg.argument, arg.description);
trace::println(_X(" %s %-*s %s"), arg.option, 29 - (int)pal::strlen(arg.option), arg.argument, arg.description);
}
trace::println(_X(" --list-runtimes Display the installed runtimes"));
trace::println(_X(" --list-sdks Display the installed SDKs"));
Expand Down
4 changes: 2 additions & 2 deletions src/native/corehost/hostmisc/pal.windows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,7 @@ bool pal::is_emulating_x64()
if (!isWow64Process2Func(GetCurrentProcess(), &pProcessMachine, &pNativeMachine))
{
// IsWow64Process2 failed. Log the error and continue.
trace::info(_X("Call to IsWow64Process2 failed: %s"), GetLastError());
trace::info(_X("Call to IsWow64Process2 failed: %u"), GetLastError());
return false;
}

Expand All @@ -821,7 +821,7 @@ bool pal::is_emulating_x64()
}

// Loading kernel32.dll failed, log the error and continue.
trace::info(_X("Could not load 'kernel32.dll': %s"), GetLastError());
trace::info(_X("Could not load 'kernel32.dll': %u"), GetLastError());

return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/native/corehost/hostpolicy/hostpolicy_context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ namespace
if (!pal::clr_palstring(path, &file_path))
{
trace::warning(_X("Failure probing contents of the application bundle."));
trace::warning(_X("Failed to convert path [%ls] to UTF8"), path);
trace::warning(_X("Failed to convert path [%hs] to UTF8"), path);

return false;
}
Expand Down
6 changes: 3 additions & 3 deletions src/native/corehost/hostpolicy/hostpolicy_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ bool hostpolicy_init_t::init(host_interface_t* input, hostpolicy_init_t* init)
// Check if there are any breaking changes.
if (input->version_hi != HOST_INTERFACE_LAYOUT_VERSION_HI)
{
trace::error(_X("The version of the data layout used to initialize %s is [0x%04x]; expected version [0x%04x]"), LIBHOSTPOLICY_NAME, input->version_hi, HOST_INTERFACE_LAYOUT_VERSION_HI);
trace::error(_X("The version of the data layout used to initialize %s is [0x%04zx]; expected version [0x%04x]"), LIBHOSTPOLICY_NAME, input->version_hi, HOST_INTERFACE_LAYOUT_VERSION_HI);
return false;
}

trace::verbose(_X("Reading from host interface version: [0x%04x:%d] to initialize policy version: [0x%04x:%d]"), input->version_hi, input->version_lo, HOST_INTERFACE_LAYOUT_VERSION_HI, HOST_INTERFACE_LAYOUT_VERSION_LO);
trace::verbose(_X("Reading from host interface version: [0x%04zx:%zd] to initialize policy version: [0x%04x:%d]"), input->version_hi, input->version_lo, HOST_INTERFACE_LAYOUT_VERSION_HI, HOST_INTERFACE_LAYOUT_VERSION_LO);

// This check is to ensure is an old hostfxr can still load new hostpolicy.
// We should not read garbage due to potentially shorter struct size
Expand All @@ -46,7 +46,7 @@ bool hostpolicy_init_t::init(host_interface_t* input, hostpolicy_init_t* init)
}
else
{
trace::error(_X("The size of the data layout used to initialize %s is %d; expected at least %d"), LIBHOSTPOLICY_NAME, input->version_lo,
trace::error(_X("The size of the data layout used to initialize %s is %zd; expected at least %d"), LIBHOSTPOLICY_NAME, input->version_lo,
offsetof(host_interface_t, host_mode) + sizeof(input->host_mode));
}

Expand Down