Closed Bug 1852662 Opened 2 years ago Closed 1 year ago

[asan perma] SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/checkouts/gecko/comm/mailnews/mime/src/mimebuf.cpp:132:10 in

Categories

(MailNews Core :: MIME, defect, P5)

Product:
MailNews Core
Component:
MIME
Type:
defect

Tracking

(thunderbird_esr128 fixed)

Status:
RESOLVED FIXED
Milestone:
132 Branch
Tracking Flags:
Tracking Status
thunderbird_esr128 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: KaiE)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Bug 1852662 - Fix UndefinedBehaviorSanitizer warning in mimebuf.cpp:132. r=BenC
48 bytes, text/x-phabricator-request
corey
: approval-comm-esr128+
Details | Review

Filed by: benc [at] thunderbird.net
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=428775495&repo=try-comm-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/U6NHU4AeS4eEPUKX1oBbqQ/runs/0/artifacts/public/logs/live_backing.log

Intermittent in the sense that it only happens on asan builds.
It's asan being paranoid again. Not really a crash risk, more a code smell we should tidy up.
This one is a result of `parse_line` function pointers of the signature:

int (parse_line)(const char line, int32_t length, MimeObject* obj);

...being passed into functions like mime_LineBuffer(), which expect a callback fn with a signature like:

int32_t (per_line_fn)(char line, uint32_t line_length, void* closure)
Summary: Intermittent SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/checkouts/gecko/comm/mailnews/mime/src/mimebuf.cpp:132:10 in → [asan perma] SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/checkouts/gecko/comm/mailnews/mime/src/mimebuf.cpp:132:10 in

Does this also affect 115, like bug 1839098?

(In reply to Wayne Mery (:wsmwk) from comment #1)

Does this also affect 115, like bug 1839098?

Flags: needinfo?(benc)

Yes, but like bug 1839098, I don't think it's serious. I don't think either of these have much risk of bad behaviour.
It's more a case of the mime code being very C ctyle (rather than C++) and doing lots of casting of types which strip away information used by asan. And asan gets all confused and complains.

So uplifting them to 115 would be more cosmetic than anything else - less asan build warnings.

Still worth fixing these things - there could be real bugs lurking in there that asan will pick up once all these not-really-a-bug ones are cleared. And besides, nobody likes any perma-orange in their treeherder!

Flags: needinfo?(benc)
Duplicate of this bug: 1889159

I have created a local patch to take care of this.
xpcshell-test now runs.
I am submitting a treeheder job right now.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=44d1af8454cef62eb509d4f22229fd7600c26831
Once I sort out any build issues on the treeserver, I will post the patch.

Assignee: nobody → ishikawa

_parse_line(const char, ...) => *_parse_line(char *, ...)
unsigned int vs int

// MimeMessage_parse_line(char *, ...)
// ISO C++11 does not allow conversion from string literal to 'char *'

ignore return value in one place

incorrect strcmp -> strcpy

treeherder job is https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f78db778597d13ff087e9ed6f7e1dab5fd116603
Looks OK to me.
In my treeherder jobs, I notice that a test, services/fxaccounts/tests/xpcshell/test_commands_closetab.js, failedl a few times even on different architecture.
The test runs fine locally under linux.
It seems one of those tests that run fine on a lightly loaded machine, but may fail on a heavily loaded machine.

I have been relying on valgrind to detect memory issues of my patches.
Unfortunately, valgrind is useless for me at this moment to check for memory issues due to Bug 1880148.
valgrind + C-C TB does not work well since it seems to me that the slowdown caused by valgrind triggers a timing-related bugin the UI libraries used by C-C TB. The context menu does not appear reliably at all. (I initially noticed it when I run C-C TB under strace to check for system call usage lately. BEFORE it worked fine. So the problem has crept into C-C TB in the last 12 months.
C-C TB cannot be tested under valgrind, and strace reliably anymore.
I suspect it may not work well under gdb well, but who knows?)

For memory problem detection, I think the next best thing is sanitized version of C-C TB. So this patch is important.

Let me explain this.
(In reply to ISHIKAWA, Chiaki from comment #29)

Created attachment 9414690 [details]
Bug 1852662 - UndefinedBehaviorSanitizer fix. r=#thunderbird-reviewers

_parse_line(const char, ...) => *_parse_line(char *, ...)

The family of _parse_line functions had the first argument qualified via "const" before the patch.
However, in one of the places where these functions are invoked after a function pointer assignment,
a function is called to do something funny to the |const char| area it is passed.
Due to the indirect call to the function defined in different files, the static check by compiler could not catch this problem(?).
(I think it is in-place encryption or something. I wonder if C-C TB worked sanely with high-level optimization after all before.)
So the |const| qualifier needs to be removed from that function, and that causes ALL the
*parse_line functions to drop |const| qualifier to execute without run-time check failure.
(Well after execution fails maybe half a times when I did a piece by piece modification to affected functions pointed out by the runtime failure, I wised up and modified all the *_parse_line functions uniformly.)

/ MimeMessage_parse_line(char *, ...)
// ISO C++11 does not allow conversion from string literal to 'char *'

That, then causes, a compilation error on tryserver in that a const string is passed MimeMesage_parse_line() in the first argument position, which now declares it to be char*.
But as the compiler on tryserver poinst out "ISO C++11 does not allow conversion from string literal to 'char *'", so this has to be modified.
(I wonder why my build did not fail locally. Maybe I am overriding the compiler option locally. Time to review the local compiler option since I have overridden the compiler option precisely because tryserver and my local build options differ before, and this is the first time in years when a build on treeherder failed due to an error that is not raised locally.)

This issue seemed to be a possible cause of a blatant run-time error and I was surprised until this is part of an error path, which may not be executed frequently at all.

unsigned int vs int

Also, before the build and execution succeeded locally, I noticed that the second argument of a few functions involved has an issue of sign vs unsigned data. I simply made the type to signed int. int32_t consistently.

ignore return value in one place
incorrect strcmp -> strcpy
Creating a patch after normal hours has its risk. In the hack to cope with "ISO C++11 does not allow conversion from string literal to 'char *'", I typed strcmpy where I should have typed strcpy. Again, I wonder why my local build did not complain.

That is it.

I hasten to add that I created a working UBSAN, ASAN version of C-C TB and ran xpcshell and mochitests successfully with the usual intermittent and perm a failures.

In the patch explanation, I forgot to mention that, in the patch, a parameter to a function declared as | void *| needs to be declared as |MimeObject *| properly and in order to do that I had to include an additional header file.

(In reply to ISHIKAWA, Chiaki from comment #31)

I hasten to add that I created a working UBSAN, ASAN version of C-C TB and ran xpcshell and mochitests successfully with the usual intermittent and perm a failures.

I may have run the xpchsell and mochitest with the ordinary binary by mistake locally. :-(

I DID check the sanitized version by running it and copy a few messages from a folder to the other (which was enough to cause this UndefinedBehavior error).

I am running xpcshell test locally using my patched SANITIZED version of C-C TB.
The test is still running, but there are a few more issues in mime-related code.
Also, I found other valid errors now, and the xpcshell test is still running.
We should have fixed this UBSAN issue much sooner.

In the updated patch, I modified the files under mime directory to take care of additional issues that I found during the execution of mochitest and xpshell-test locally.

I have found three other problems under different directories, but this should get the SANITIZE version going for much longer before terminated due to UndefinedBehavior.

I posted a slightly updated patch that incorporates the bitrot fix which is caused by the disagreement of clang-format on try-comm-central and local environment lately. I think it was OK a couple of months ago, but somehow I noticed the placement of "*" to signify the pointer reference has changed somehow.

In the try builds that were posted here, apparently none included an ASAN build.
Do we know if the suggested patch actually fixed the ASAN issue?

I'm experimenting with a potential alternative patch, try build here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6235a8e6961c22ddad9bf2b2228389de832d993e
but it still shows a failure.

My patch still had a mismatch between uint32 and int32.
Trying to fix that...

My experiment fixed the warning in the reported place.
But that wasn't sufficient, now I get a similar warning at another place.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a5fbaa6f01ddc0062fac894f114dbe884b14404c

I suggest to fix this step by step. The patch from that try build fixes the originally reported issue. I suggest we file new bugs for the additional reported places.

I did some quick experiments, but I couldn't solve the additional places yet using a similar approach. It wasn't yet completely clear to me what types I should use instead of void*. More investigation is needed.

Attachment #9425008 - Attachment description: WIP: Bug 1852662 - Fix UndefinedBehaviorSanitizer warning in mimebuf.cpp. → Bug 1852662 - Fix UndefinedBehaviorSanitizer warning in mimebuf.cpp:132. r=BenC

(In reply to Kai Engert (:KaiE:) from comment #38)

In the try builds that were posted here, apparently none included an ASAN build.
Do we know if the suggested patch actually fixed the ASAN issue?

I'm experimenting with a potential alternative patch, try build here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6235a8e6961c22ddad9bf2b2228389de832d993e
but it still shows a failure.

Aha, we can use linux64-asan as the tryserver target.
I wish this is documented somewhere.

Hmm.
Do we need a special tweaking for try-comm-central submission?
I followed the "try: ..." syntax used for the submission mentioned in comment 41, but my try submission did not seem to generate
the ASAN build and run tests.

E.g.
try: -b do -p linux64-asan -u all -t none --setenv MOZ_LOG=TBFileIO:5,MsgDB:5,timestamp
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=88c570ab11bb70f7224e2d54d6cb12f2f3f12af2

try: -b do -p linux64-asan -u all --setenv MOZ_LOG=TBFileIO:5,MsgDB:5,timestamp
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a1d431cc3bf90fdb77de1f63b4216c97fc7f7a32

I suspecting the undocumented "--setenv ..." might have interfered the parsing of linux64-asan, I took it out. Still to no avail.
try: -b o -p linux64-asan -u all
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=09bd0c6a682096501dc51133134f99333a2d99a7

The patch I posted here created an ASAN binary which worked just fine on my local linux PC.
Only in debug build, it generated an ASAN problem after the main execution finished and GC was invoked for cleanup.
The problem happened with about 1/30 probability. The error happened when I printed the database cache information (for debugging purposes).
But in optimized build, it should run without any issue.

(In reply to ISHIKAWA, Chiaki from comment #43)

Aha, we can use linux64-asan as the tryserver target.

No, I tried that, but it didn't work.

To get the ASAN build, I used the treeherder user interface to add the ASAN build.
You need to be logged in (upper right), then you can lick the little arrorow on the top right of the build.
Select "add new jobs".
That changes what's shown on screen, and new build options are added and shown in yellow.
I clicked on the following:

  • Linux x64 asan: Bo
  • Linux 18.04 x64 WebRender asan opt, there are two sections M and X. I clicked on the + inside each of those. Then I clicked on the "all" inside each of those.
  • now on the top right, there was a button "trigger N new jobs"

Done. It will take a few minutes before the new ASAN build appears

Keywords: checkin-needed-tb

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/6df02b94abf1
Fix UndefinedBehaviorSanitizer warning in mimebuf.cpp:132. r=BenC

Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
status-thunderbird_esr128: --- → affected
Target Milestone: --- → 132 Branch

(In reply to Kai Engert (:KaiE:) from comment #45)

(In reply to ISHIKAWA, Chiaki from comment #43)

Aha, we can use linux64-asan as the tryserver target.

No, I tried that, but it didn't work.

To get the ASAN build, I used the treeherder user interface to add the ASAN build.
You need to be logged in (upper right), then you can lick the little arrorow on the top right of the build.
Select "add new jobs".
That changes what's shown on screen, and new build options are added and shown in yellow.
I clicked on the following:

  • Linux x64 asan: Bo
  • Linux 18.04 x64 WebRender asan opt, there are two sections M and X. I clicked on the + inside each of those. Then I clicked on the "all" inside each of those.
  • now on the top right, there was a button "trigger N new jobs"

Done. It will take a few minutes before the new ASAN build appears

Thank you for the pointer.

But when I tried the above (the little arrow on the top of the build is an inverted triangle of a sorton my web browser),
I got the following error(s).
At one time,

Taskcluster: Client ID mozilla-auth0/email|58878bc06575212047e3e163/treeherder-production-client-eSL02e does not have sufficient scopes and is missing the following scopes: ``` hooks:trigger-hook:project-comm/in-tree-action-1-generic/3855bf7d13 ``` This request requires the client to satisfy the following scope expression: ``` hooks:trigger-hook:project-comm/in-tree-action-1-generic/3855bf7d13 ``` --- * method: triggerHook * errorCode: InsufficientScopes * statusCode: 403 * time: 2024-09-17T10:08:03.784Z

at another time,

Taskcluster: Client ID mozilla-auth0/email|58878bc06575212047e3e163/treeherder-production-client-eSL02e does not have sufficient scopes and is missing the following scopes: ``` hooks:trigger-hook:project-comm/in-tree-action-1-generic/3855bf7d13 ``` This request requires the client to satisfy the following scope expression: ``` hooks:trigger-hook:project-comm/in-tree-action-1-generic/3855bf7d13 ``` --- * method: triggerHook * errorCode: InsufficientScopes * statusCode: 403 * time: 2024-09-17T10:10:37.193Z

It seems that I don't have the proper scope for operation? (I have logged in.)

Component: General → MIME
Product: Thunderbird → MailNews Core
Blocks: 1919290
Blocks: 1921950
Assignee: ishikawa → kaie

Comment on attachment 9425008 [details]
Bug 1852662 - Fix UndefinedBehaviorSanitizer warning in mimebuf.cpp:132. r=BenC

[Approval Request Comment]
Regression caused by (bug #): not a regression
User impact if declined: potential crashes not fixed
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low

Attachment #9425008 - Flags: approval-comm-esr128?

Given the size of this bug and related bug 1919290, you could consider postponing to a 128.4.1 release (not 128.4.0).

The patch applies cleanly on comm-esr128 for me, please ping me if you have any merge issues, and I'll upload mine.

Let's postpone until we have more clarify around 1925085, there are some overlaps in patches, let's avoid merging trouble for now.

(In reply to Kai Engert (:KaiE:) from comment #45)

(In reply to ISHIKAWA, Chiaki from comment #43)

Aha, we can use linux64-asan as the tryserver target.

No, I tried that, but it didn't work.

To get the ASAN build, I used the treeherder user interface to add the ASAN build.
You need to be logged in (upper right), then you can lick the little arrorow on the top right of the build.
Select "add new jobs".
That changes what's shown on screen, and new build options are added and shown in yellow.
I clicked on the following:

  • Linux x64 asan: Bo
  • Linux 18.04 x64 WebRender asan opt, there are two sections M and X. I clicked on the + inside each of those. Then I clicked on the "all" inside each of those.
  • now on the top right, there was a button "trigger N new jobs"

Done. It will take a few minutes before the new ASAN build appears

Initially, I could not invoke ASAN build by following the steps you suggested.

Obviously my mozilla account was set up incorrectly.
After a few exchanges in #matrix and MANY e-mail exchanges with a kind support person, I finally got the account sorted out, and
I could invoke asan build (!)
Thank you again for posting the steps to do that.

I should have realized that something was amiss many years ago when I could not cancel my try-comm-central submission after I realized the submission did not include the correct patch. I was so clueless.

OTOH, I do think we should be able to specify ASAN build when we use the command line to submit a job to try server.
(Bug 1920649)

Attachment #9414690 - Attachment is obsolete: true

Comment on attachment 9425008 [details]
Bug 1852662 - Fix UndefinedBehaviorSanitizer warning in mimebuf.cpp:132. r=BenC

[Triage Comment]
Approved for esr128

Attachment #9425008 - Flags: approval-comm-esr128? → approval-comm-esr128+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: