Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade MUSL Version #7279

Open
EricWF opened this issue Oct 13, 2018 · 14 comments
Open

Upgrade MUSL Version #7279

EricWF opened this issue Oct 13, 2018 · 14 comments

Comments

@EricWF
Copy link
Contributor

EricWF commented Oct 13, 2018

Hi All,

I'm wondering if there are any plans to upgrade the MUSL libc version? There are a number of printf bugs that have been fixed since 1.1.15.

I tried upgrading it myself but it's non-trivial and there is very little information about the steps needed to do so. I would be willing to try again if I can be provided with a little direction.

@kripken
Copy link
Member

kripken commented Oct 14, 2018

I added some docs for how to do that now, see #7286. Let me know if that helps or if anything still isn't clear.

@stale
Copy link

stale bot commented Oct 14, 2019

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Oct 14, 2019
@sbc100
Copy link
Collaborator

sbc100 commented Oct 14, 2019

We should do this.

@stale stale bot removed the wontfix label Oct 14, 2019
@nhooyr
Copy link

nhooyr commented Mar 3, 2020

Please do.

if (((uintptr_t)s & ALIGN) == ((uintptr_t)d & ALIGN)) {
has caused issues for us as it will read out of bounds when it tries to read 4 bytes at a time. Latest muslc has fixed this: https://git.musl-libc.org/cgit/musl/tree/src/string/strlcpy.c

@kripken
Copy link
Member

kripken commented Apr 14, 2020

@nhooyr we updated that file specifically now to fix that issue.

A full update of musl is still very important to do, hopefully someone volunteers to take the time soon!

@kleisauke
Copy link
Collaborator

+1 for full update of musl. fwiw, it looks like stpncpy is also affected for the above mentioned bug.

Details
=================================================================
==42==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0226c8e8 at pc 0x009ebc59 bp 0x035abbb0 sp 0x035abbbc
READ of size 4 at 0x0226c8e8 thread T0
    #0 0x9ebc59 in __stpncpy+0x9ebc59 (http://localhost:5000/lib/vips.wasm+0x9ebc59)
    #1 0x9ea6aa in strncpy+0x9ea6aa (http://localhost:5000/lib/vips.wasm+0x9ea6aa)

0x0226c8e8 is located 56 bytes to the left of global variable '<string literal>' defined in 'exif-entry.c:680:27' (0x226c920) of size 10
  '<string literal>' is ascii string 'Top-right'
0x0226c8e9 is located 0 bytes to the right of global variable '<string literal>' defined in 'exif-entry.c:680:11' (0x226c8e0) of size 9
  '<string literal>' is ascii string 'Top-left'
SUMMARY: AddressSanitizer: global-buffer-overflow (http://localhost:5000/lib/vips.wasm+0x9ebc58) in __stpncpy+0x9ebc58
Shadow bytes around the buggy address:
  0x0044d8c0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0044d8d0: f9 f9 f9 f9 00 06 f9 f9 f9 f9 f9 f9 00 06 f9 f9
  0x0044d8e0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 00 00 03
  0x0044d8f0: f9 f9 f9 f9 00 00 00 03 f9 f9 f9 f9 00 00 00 05
  0x0044d900: f9 f9 f9 f9 00 00 00 05 f9 f9 f9 f9 00 00 01 f9
=>0x0044d910: f9 f9 f9 f9 00 00 00 07 f9 f9 f9 f9 00[01]f9 f9
  0x0044d920: f9 f9 f9 f9 00 02 f9 f9 f9 f9 f9 f9 00 05 f9 f9
  0x0044d930: f9 f9 f9 f9 00 04 f9 f9 f9 f9 f9 f9 00 01 f9 f9
  0x0044d940: f9 f9 f9 f9 00 02 f9 f9 f9 f9 f9 f9 00 05 f9 f9
  0x0044d950: f9 f9 f9 f9 00 04 f9 f9 f9 f9 f9 f9 00 01 f9 f9
  0x0044d960: f9 f9 f9 f9 00 01 f9 f9 f9 f9 f9 f9 00 06 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==42==ABORTING

(memccpy, memchr, memmove, stpcpy, strchrnul and strlen are probably also affected. See this commit)

@jeromelaban
Copy link

jeromelaban commented Nov 27, 2020

I wonder, I see that @kripken has updated many of the MUSL in #11215, yet I'm still getting the same buffer overrun issue mentioned in #7279 (comment) when using asan with strncpy.

Would someone know more about this ?

The overrun happens for this code, for instance:

char test[] = "12345";
auto target = (char*)malloc(150);
strncpy(target, test, 150);

as the destination size is larger, forcing the aligned copy loop to read out of bounds of the test buffer.

A workaround can be to define a method that will override the weak alias of strncpy, without the aligned copy behavior:

char *strncpy(char * d, const char * s, size_t n)
{
	for (; n && (*d=*s); n--, s++, d++);
	memset(d, 0, n);
	return d;
}

@sbc100
Copy link
Collaborator

sbc100 commented Nov 28, 2020

Does your example repro under asan? if not how have your been detecting it? It would be good to have failing test so we can be sure we've fixed it.

@jeromelaban
Copy link

Yes it does with asan enabled, here's how I reproduced it:

emcc \
    test.cpp \
    -fsanitize=address -s TOTAL_MEMORY=393216000 \
    -s DISABLE_EXCEPTION_CATCHING=0 \
    --profiling \
    -o index.html
#include <stdio.h>
#include <emscripten.h>
#include <stdlib.h>
#include <string.h>

int main() {
    char test[] = "12345";

    auto target = (char*)malloc(150);
    strncpy(target, test, 150);
    printf("target: %s\n", target);

    return 0;
}

With the following error:

=================================================================
==42==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x03845eb4 at pc 0x0000106f bp 0x03845e90 sp 0x03845e9c
READ of size 4 at 0x03845eb4 thread T0
    #0 0x106f in __stpncpy+0x106f (http://localhost:8000/index.wasm+0x106f)

Address 0x03845eb4 is located in stack of thread T0 at offset 20 in frame
    #0 0x11 in setTempRet0+0x11 (http://localhost:8000/index.wasm+0x11)

  This frame has 1 object(s):
    [16, 22) 'test' <== Memory access at offset 20 partially overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (http://localhost:8000/index.wasm+0x106e) in __stpncpy+0x106e
Shadow bytes around the buggy address:
  0x00708b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708ba0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708bb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708bc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x00708bd0: 00 00 00 00 f1 f1[06]f3 f3 f3 00 00 00 00 00 00
  0x00708be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00708c20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==42==ABORTING

Where would be the best location to add this repro in tests ?

@sbc100
Copy link
Collaborator

sbc100 commented Nov 28, 2020

I think you could add this alongside test_strndup.c in test_core.py.

In order to run that test with asan you would run ./tests/runner.py asan.test_strndup.

@jeromelaban
Copy link

#12905 should do. It fails when running the test locally.

@kleisauke
Copy link
Collaborator

I started working to update musl to version 1.2.1 (i.e. latest version as the time of writing), see:
master...kleisauke:musl-1.2.1

It already passes the test suite locally, so expect a PR soon.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 10, 2020

I also had a go at the musl upgrade last week: #13006. Happy to go with your version if they are basically equivalent.

sbc100 added a commit that referenced this issue Jan 18, 2021
This allows us to then remove some of our emscripten-specific
arch headers that are not longer needed (since they do not
differ from the generic version).

This is part of making musl updates easier.

See #7279
sbc100 added a commit that referenced this issue Jan 18, 2021
This allows us to then remove some of our emscripten-specific
arch headers that are not longer needed (since they do not
differ from the generic version).

This is part of making musl updates easier.

See #7279
sbc100 added a commit that referenced this issue Jan 18, 2021
This allows us to then remove some of our emscripten-specific
arch headers that are not longer needed (since they do not
differ from the generic version).

This is part of making musl updates easier.

See #7279
sbc100 added a commit that referenced this issue Jan 19, 2021
This allows us to then remove some of our emscripten-specific
arch headers that are not longer needed (since they do not
differ from the generic version).

This is part of making musl updates easier.

See #7279
sbc100 added a commit that referenced this issue Jan 19, 2021
This allows us to then remove some of our emscripten-specific
arch headers that are not longer needed (since they do not
differ from the generic version).

This is part of making musl updates easier.

See #7279
@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants