Closed Bug 1340755 Opened 7 years ago Closed 7 years ago

Port bug 1060419 to mailnews: fix arguments to printf format strings in c-c

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 54.0

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(1 file, 1 obsolete file)

patch v2
30.87 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review 
New breakage incoming from m-c, seen in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=128fe29858058424d1fe7582e5e70690c5bf3005 

Looks like m-c tightened printf format string argument checks:

/builds/slave/tb-try-c-cen-lx-00000000000000/build/mailnews/import/src/nsImportMail.cpp:774:255: error: format '%s' expects argument of type 'char*', but argument 4 has type 'const char_type* {aka const char16_t*}' [-Werror=format=]
/builds/slave/tb-try-c-cen-lx-00000000000000/build/mailnews/import/src/nsImportMail.cpp:784:259: error: format '%s' expects argument of type 'char*', but argument 4 has type 'const char_type* {aka const char16_t*}' [-Werror=format=]
/builds/slave/tb-try-c-cen-lx-00000000000000/build/mailnews/import/src/nsImportMail.cpp:825:256: error: format '%s' expects argument of type 'char*', but argument 4 has type 'const char_type* {aka const char16_t*}' [-Werror=format=]
/builds/slave/tb-try-c-cen-lx-00000000000000/build/mailnews/local/src/nsMovemailService.cpp:103:189: error: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'nsMovemailService*' [-Werror=format=]
/builds/slave/tb-try-c-cen-lx-00000000000000/build/mailnews/db/msgdb/src/nsMsgDatabase.cpp:975:232: error: format '%ld' expects argument of type 'long int', but argument 5 has type 'unsigned int' [-Werror=format=]
/builds/slave/tb-try-c-cen-lx-00000000000000/build/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1212:142: error: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Werror=format=]
Component: Import → Backend
Summary: fix arguments to %s format string in nsImportMail.cpp → fix arguments to printf format strings in c-c
Attached patch patch (obsolete) — — Splinter Review 
I've got this far and I pass on the relay if needed :)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=13103a5327de2cd5e59c65971c20d6bba25039a2

I'm not very happy about the uint32_t(rv) casts, but is there a nice function converting nsresult to uint32_t or similar? There are some occurences of this like https://dxr.mozilla.org/comm-central/source/mozilla/dom/media/MediaShutdownManager.cpp#82 .
Attachment #8838821 - Flags: review?(jorgk)
See bug 1060419 comment #593, landed 36 changesets (if I didn't miscount).

I don't know why that happens always on a weekend:
Atoms:   Bug 1334558 - 2017-01-27 19:13:54 CET (Friday)
Proxies: Bug 1336789 - 2017-02-05 11:04:36 CET (Sunday)

Looks like it comes from here:
https://hg.mozilla.org/mozilla-central/rev/5d287c298e2e#l1.13

Can't we just switch this off as a first step?
Summary: fix arguments to printf format strings in c-c → Port bug 1060419 to mailnews: fix arguments to printf format strings in c-c
Comment on attachment 8838821 [details] [diff] [review]
patch

Review of attachment 8838821 [details] [diff] [review]:
-----------------------------------------------------------------

So the changes are:
%p for pointers (this)
uint32_t(rv)
various length adjustments, like %llx<->%lx, %lx<->%x, %ld<->%d
use of %u for unsigned integers.

Can you carry on here. I'm afraid on Windows anything goes, so I won't get (some of) those errors. Sadly you cancelled the try, so we won't know.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1457,5 @@
>    }
> +
> +  nsCString hostname;
> +  rv = smtpServer->GetHostname(hostname);
> +  NS_ENSURE_SUCCESS(rv, rv);

Remove error checking. We don't want to change the behaviour here.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +1843,5 @@
>        rv = imapMailFolderSink->CopyNextStreamMessage(GetServerStateParser().LastCommandSuccessful() &&
>                                                  NS_SUCCEEDED(GetConnectionStatus()),
>                                                  copyState);
>        if (NS_FAILED(rv))
> +        MOZ_LOG(IMAP, LogLevel::Info, ("CopyNextStreamMessage failed: %x\n", int32_t(rv)));

uint32_t like in the other ones.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1388,5 @@
>      return 0;
>    }
>  
>    m_pop3ConData->next_state = POP3_ERROR_DONE;
> +  MOZ_LOG(POP3LOGMODULE, LogLevel::Info, (POP3LOG("Pop3SendData failed: %x"), int32_t(result)));

uint32_t like in the other ones.
Attachment #8838821 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+1) from comment #4)
> Can you carry on here. I'm afraid on Windows anything goes, so I won't get
> (some of) those errors. Sadly you cancelled the try, so we won't know.
I can still build, with and without that patch. Richard, how is your build going this morning?
Flags: needinfo?(richard.marti)
My Windows build was okay. But https://hg.mozilla.org/mozilla-central/rev/5d287c298e2e#l1.13 seems to affect only Linux and macOS.
Flags: needinfo?(richard.marti)
My macOS build finished now without the patch. Probably I need to clobber to trigger the errors.
Strange, also a clobber build was successful without patch on Mac.
I think I am moving in circles now. What passes on 64bit linux does not pass on 32bit linux as the interpretation of the %l modifiers differs. I need to look at the m-c patches.
Thanks for looking into this while I'm RAM shopping and destroying l10n builds on Mac :-(
Your Linux64 builds are green, so that's certainly progress, it is not?
Also good that the patch doesn't appear to be huge ... so far.
It is green because that is what I test on locally. But I think it broke 32bit linux now in places that weren't broken before. It seems I need to use the %PRIu32 formats now (instead of the ambiguous %lu, %u, etc).
Well, in the 36 changesets they landed, these are the largest ones with many examples:
https://hg.mozilla.org/mozilla-central/rev/9eab13b3a19a
https://hg.mozilla.org/mozilla-central/rev/495b8a307555

-         ("HttpAsyncAborter::AsyncAbort [this=%p status=%x]\n", mThis, status));
+         ("HttpAsyncAborter::AsyncAbort [this=%p status=%" PRIx32 "]\n",
+          mThis, static_cast<uint32_t>(status)));

-  LOG(("HttpChannelChild::Suspend [this=%p, mSuspendCount=%lu, "
+  LOG(("HttpChannelChild::Suspend [this=%p, mSuspendCount=%" PRIu32 ", "
        "mDivertingToParent=%d]\n", this, mSuspendCount+1, mDivertingToParent));

-  LOG(("OBJLC [%p]: Notifying about state change: (%u, %llx) -> (%u, %llx)"
+  LOG(("OBJLC [%p]: Notifying about state change: (%u, %" PRIx64 ") -> (%u, %" PRIx64 ")"

But you found those yourself :-)
Attached patch patch v2 — — Splinter Review 
It seems we didn't have as many problems as m-c :)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9840b41148983ba6e436bf5dc784b2f4c62f6e7d
Attachment #8838821 - Attachment is obsolete: true
Attachment #8838886 - Flags: review?(jorgk)
Comment on attachment 8838886 [details] [diff] [review]
patch v2

Review of attachment 8838886 [details] [diff] [review]:
-----------------------------------------------------------------

Great. You missed one. Please mention bug 1060419 in the commit message:
Bug 1340755 - fix format string arguments in mailnews after bug 1060419. r=jorgk

It's not about 'printf'.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +534,5 @@
>    // ignore errors handling the QUIT command so fcc can continue.
>    if (m_sendDone && NS_FAILED(aStatus))
>    {
>      MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Info,
> +     ("SMTP connection error quitting %" PRIx32 ", ignoring ", uint32_t(aStatus)));

cast missing.
Comment on attachment 8838886 [details] [diff] [review]
patch v2

Forgot the r+
Attachment #8838886 - Flags: review?(jorgk) → review+
Actually:
Bug 1340755 - fix format specifiers in mailnews after bug 1060419. r=jorgk
OK, pushing.
https://hg.mozilla.org/comm-central/rev/541879fbcbe89ae7827850d6807e17ca95013eb2
Thanks.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Fwiw, on OpenBSD/clang this fails (c-c was also previously failing but not on this spot):

49:13.69 /build/buildslave-amd64/comm-central-amd64/build/mailnews/local/src/nsMovemailService.cpp:556:36: error: expected ')'
49:13.69   LOG(("GetNewMail returning rv=%" PRIx32, static_cast<uint32_t>(rv)));
49:13.69                                    ^
49:13.69 /build/buildslave-amd64/comm-central-amd64/build/mailnews/local/src/nsMovemailService.cpp:556:3: note: to match this '('
49:13.69   LOG(("GetNewMail returning rv=%" PRIx32, static_cast<uint32_t>(rv)));
49:13.69   ^
49:13.69 /build/buildslave-amd64/comm-central-amd64/build/mailnews/local/src/nsMovemailService.cpp:46:19: note: expanded from macro 'LOG'
49:13.69 #define LOG(args) MOZ_LOG(gMovemailLog, mozilla::LogLevel::Debug, args)
49:13.69                   ^
49:13.69 /build/obj/buildslave/comm-central/dist/include/mozilla/Logging.h:244:33: note: expanded from macro 'MOZ_LOG'
49:13.69       mozilla::detail::log_print(_module, _level, MOZ_LOG_EXPAND_ARGS _args);   

Investigating various things, including clobber build, including sizeprintfmacros.h, etc...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: