1919290 - Multiple ASAN failure reports in comm/mailnews/mime (mimeenc.cpp:162:12, mimeenc.cpp:287:12, mimeenc.cpp:489:18, mimepbuf.cpp:207:14)

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Kai Engert [:KaiE:]]

After fixing bug 1852662, the ASAN UndefinedBehaviorSanitizer reports multiple additional failures.

The root cause of these issues is:
Our MIME C code uses an untyped void* in various places, but some functions use an explicit typed parameter.

We could potentially fix this new ASAN warning simply by changing those functions to also take a void* paramter, and do an optimistic type cast.

But I think this is against the idea of these checks. We shouldn't fix type checking by forcing parameters to be untyped, and then perform risky downcasting.

My suggestion is that we change the void* for all the "closure" parameters to a lightweight wrapper type, which can be passed around as a parameter (without requiring allocation).

The wrapper should transport which type of parameter we're transporting. That allows our casting functions to perform a check for the expected type, and safely abort on failure (instead of potentially crashing).

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Bug 1919290 - Type checking for an initial set of closure parameters in C MIME code. r=mkmelin

Comment 2

[Kai Engert [:KaiE:]]

Attachment: Bug 1919290 - Type checking for data_object in C MIME code. r=BenC

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Bug 1919290 - Type checking for output and image closure parameters in C MIME code. r=mkmelin

Comment 4

[Wayne Mery (:wsmwk)]

Will this also impact 128?

Comment 5

[Kai Engert [:KaiE:]]

Yes, I think we should uplift these fixes to 128.

Comment 6

[Kai Engert [:KaiE:]]

The patch here depends on the following two commits from bug 1915397,
if you don't apply them (on esr128), you get merge issues.

https://hg.mozilla.org/comm-central/rev/c306f1b7067d
https://hg.mozilla.org/comm-central/rev/306a51c14688

Comment 7

[Kai Engert [:KaiE:]]

Attachment: Bug 1919290 - Use smarter logic to avoid code repetition. r=BenC

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a9a403a53dcc
Type checking for an initial set of closure parameters in C MIME code. r=mkmelin
https://hg.mozilla.org/comm-central/rev/3fdd12e6840c
Type checking for data_object in C MIME code. r=BenC
https://hg.mozilla.org/comm-central/rev/2f426872727b
Type checking for output and image closure parameters in C MIME code. r=mkmelin
https://hg.mozilla.org/comm-central/rev/b492a2fe9f56
Use smarter logic to avoid code repetition. r=BenC

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 9425307 [details]
Bug 1919290 - Type checking for an initial set of closure parameters in C MIME code. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: these patches improve correctness and could potentially reduce risk for crashes
Testing completed (on c-c, etc.): should have no functional changes for the user
Risk to taking this patch (and alternatives if risky): small risk for accidental change in behavior of processing messages

Comment 10

[Kai Engert [:KaiE:]]

Note that the 4th patch undoes some of the earlier changes, and modifies them to shorter code. I will attach a patch FYI, that shows that the change is smaller.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: illustrative-1919290-combined.patch

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 9425307 [details]
Bug 1919290 - Type checking for an initial set of closure parameters in C MIME code. r=mkmelin

Let's wait a little longer with 128. I realize we'll also need bug 1852662, which got a follow-up bug 1921950, which I'd like to investigate first.

Comment 13

[Corey Bryant]

Comment on attachment 9425307 [details]
Bug 1919290 - Type checking for an initial set of closure parameters in C MIME code. r=mkmelin

[Triage Comment]
Approved for beta
There are 4 patches in total

Comment 14

[Corey Bryant]

Comment on attachment 9426423 [details]
Bug 1919290 - Type checking for data_object in C MIME code. r=BenC

[Triage Comment]
Approved for beta

Comment 15

[Corey Bryant]

Comment on attachment 9426424 [details]
Bug 1919290 - Type checking for output and image closure parameters in C MIME code. r=mkmelin

[Triage Comment]
Approved for beta

Comment 16

[Corey Bryant]

Comment on attachment 9428172 [details]
Bug 1919290 - Use smarter logic to avoid code repetition. r=BenC

[Triage Comment]
Approved for beta

Comment 17

[Corey Bryant]

Thunderbird 132.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/cd03b92cd21d
https://hg.mozilla.org/releases/comm-beta/rev/84b51acb9365
https://hg.mozilla.org/releases/comm-beta/rev/6ca6b16fb214
https://hg.mozilla.org/releases/comm-beta/rev/7e42c05308f4

Comment 18

[Kai Engert [:KaiE:]]

Comment on attachment 9425307 [details]
Bug 1919290 - Type checking for an initial set of closure parameters in C MIME code. r=mkmelin

[Approval Request Comment]
see comment 9

Comment 19

[Kai Engert [:KaiE:]]

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

Comment 20

[Kai Engert [:KaiE:]]

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

Comment 21

[Kai Engert [:KaiE:]]

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