Closed Bug 1243724 Opened 5 years ago Closed 5 years ago

firefox 44 crashes due to depending on undefined behavior with memcpy()

Categories

(Core :: Networking: HTTP, defect)

44 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1224291

People

(Reporter: natanael.copa, Unassigned)

References

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

built firefox 44.0 on x86_64 alpine linux (which uses musl libc) and opened http://streambuilder.pro/docs/how-tos/check-iqsv/


Actual results:

fortify check caught bad use of memcpy() and crashed.

This does not happen with firefox-43.

What happens is that brotli is built without BROTLI_BUILD_PORTABLE by default.

from: modules/brotli/dec/port.h:
#ifdef BROTLI_BUILD_PORTABLE
#define BROTLI_ALIGNED_READ 1
#define BROTLI_SAFE_MEMMOVE 1
#else
#define BROTLI_ALIGNED_READ 0
#define BROTLI_SAFE_MEMMOVE 0
#endif

So we get BROTLI_SAFE_MEMMOVE=0. This will select the following code in 
modules/brotli/dec/decode.c:

static BROTLI_INLINE BROTLI_NO_ASAN void memmove16(
    uint8_t* dst, uint8_t* src) {
#if BROTLI_SAFE_MEMMOVE
  /* For x86 this compiles to the same binary as signle memcpy.
     On ARM memcpy is not inlined, so it works slower.
     This implementation makes decompression 1% slower than regular one,
     and 2% slower than NEON implementation.
   */
  uint32_t buffer[4];
  memcpy(buffer, src, 16);
  memcpy(dst, buffer, 16);
#elif defined(__ARM_NEON__)
  vst1q_u8(dst, vld1q_u8(src));
#else
  /* memcpy is unsafe for overlapping regions and ASAN detects this.
     But, because of optimizations, it works exactly as memmove:
     copies data to registers first, and then stores them to dst. */
  memcpy(dst, src, 16);
#endif
}



and we end up using memcpy() in a way that depends on how implementation deals with overlapping memory regions. FORTIFY_SOURCE on alpine linux did not like this and went boom.




Expected results:

pages should have open without crashing, regardless of how memcpy() is implemented.
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
I think it would make sense to build brotli with BROTLI_BUILD_PORTABLE by default, and possibly have a whitelist of platforms where you know for sure that it can be disabled.
Moving to Core: Networking and marking this crash blocking the brotli implementation.
Blocks: 366559
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Credit to musl libc developer Szabolcs Nagy, who helped analyze the disassembly.
This is basically the same report as bug 1224291, except that there it was reported by ASAN.

I guess we should go ahead and take the patch in that bug, to avoid this issue.
Depends on: 1224291
Fixed in upstream
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1224291
You need to log in before you can comment on or make changes to this bug.