1901700 - Crash in [@ hwy::N_AVX3::LoadU]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

7 crashes from 5 installs of 128.0a1 and 129.0a1 on Windows. libjxl had been updated on June 7 in bug 1900670.

Kagami, please evaluate this bug.

Crash report: https://crash-stats.mozilla.org/report/index/18f5cb97-9cb2-4167-9944-1cb630240611

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  hwy::N_AVX3::LoadU(hwy::N_AVX3::Simd<float, 16, 0>, float const*)  third_party/jpeg-xl/lib/jxl/render_pipeline/stage_gaborish.cc:68
0  xul.dll  jxl::N_AVX3::GaborishStage::ProcessRow(std::vector<std::vector<float*, std::a...  third_party/jpeg-xl/lib/jxl/render_pipeline/stage_gaborish.cc:71
1  xul.dll  jxl::LowMemoryRenderPipeline::RenderRect(unsigned long long, std::vector<jxl:...  third_party/jpeg-xl/lib/jxl/render_pipeline/low_memory_render_pipeline.cc:725
2  xul.dll  jxl::LowMemoryRenderPipeline::ProcessBuffers(unsigned long long, unsigned lon...  third_party/jpeg-xl/lib/jxl/render_pipeline/low_memory_render_pipeline.cc:892
3  xul.dll  jxl::RenderPipeline::InputReady(unsigned long long, unsigned long long, std::...  third_party/jpeg-xl/lib/jxl/render_pipeline/render_pipeline.cc:121
3  xul.dll  jxl::RenderPipelineInput::Done()  third_party/jpeg-xl/lib/jxl/render_pipeline/render_pipeline.cc:135
4  xul.dll  jxl::FrameDecoder::ProcessACGroup(unsigned long long, jxl::BitReader**, unsig...  third_party/jpeg-xl/lib/jxl/dec_frame.cc:547
5  xul.dll  jxl::FrameDecoder::ProcessSections::<lambda_2>::operator()(unsigned long long...  third_party/jpeg-xl/lib/jxl/dec_frame.cc:700
5  xul.dll  jxl::ThreadPool::RunCallState<`lambda at /builds/worker/checkouts/gecko/third...  third_party/jpeg-xl/lib/jxl/base/data_parallel.h:94
6  xul.dll  jpegxl::ThreadParallelRunner::RunRange(jpegxl::ThreadParallelRunner*, unsigne...  third_party/jpeg-xl/lib/threads/thread_parallel_runner_internal.cc:145

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Without a testcase I can't say much, but I can try updating highway and see if it helps.

Comment 2

[Timothy Nikkel (:tnikkel)]

jpeg xl is nightly only and presumably one has to enable the jpegxl pref for this too, so S3.

Comment 5

[Daniel Veditz [:dveditz]]

I tagged this "csectype-wildptr", but given the crashing instruction it could be just a plain bounds issue. Do we test this code under ASAN?

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Veditz [:dveditz] from comment #5)

Do we test this code under ASAN?

There are some jpegxl reftests, gtests, and webplatform tests. I'm not sure which suites we regularly run under asan.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:saschanaz, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Should this remain sec-high even if it's disabled by default and included only on nightly (non-nightly cannot enable this) ?

Comment 11

[Daniel Veditz [:dveditz]]

It's sec-high with a status of "disabled". If we downrate the security severity there is nothing that would trigger us to uprate it again if we change the build such that this feature makes it into release. This is not a common situation thus the bugbot comment, but I agree this is definitely not an "S2" bug as long as it's disabled.

Comment 12

[Daniel Veditz [:dveditz]]

Maybe you have a point; JXL is in a weird limbo. It exists, but doesn't seem like we plan to ship it. Is there a master JXL bug this could block?

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

I don't see a sec bug there, but I don't have a permission to see one either. We can try pinging them if needed.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

Looks like this is still happening after lib update, I'll ping the authors.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

So Google pointed out it's fixed a few days ago but one shouldn't be affected by this with tagged releases. So far we have been sticking with the main branch, we should stop that. That's bug 1904318.

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

Wait why the signature change? Can't roll it back because it's not fully shown in comment #16 😬

Comment 19

[Kagami Rosylight [:saschanaz] (they/them)]

It's gone after bug 1904318.