mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <dalias@libc.org>
To: Pablo Correa Gomez <ablocorrea@hotmail.com>
Cc: musl@lists.openwall.com
Subject: Re: [musl] newlocale: Segmentation fault when locale input is NULL
Date: Wed, 6 Oct 2021 08:29:37 -0400	[thread overview]
Message-ID: <20211006122936.GD2559@brightrain.aerifal.cx> (raw)
In-Reply-To: <AM5P192MB00812237500EA1FEBEB2C7ABC7B09@AM5P192MB0081.EURP192.PROD.OUTLOOK.COM>

On Wed, Oct 06, 2021 at 11:31:29AM +0200, Pablo Correa Gomez wrote:
> Dear musl maintainers,
> 
> While doing some work in GNOME control center for postmarketos, we
> bumped into a segmentation fault which is also present in GNOME in
> Alpine[1].
> 
> After doing some degugging, I figured out that the reason is that,
> through GNOME desktop[2], there is a call to newlocale, where they end
> up calling it with a NULL argument.
> 
> newlocale(LC_CTYPE, NULL, (locale_t)0);
> 
> In this case, "name" is passed to __get_locale in
> src/locale/newlocale.c:27 and then dereferenced in
> src/locale/locale_map.c:43, causing a segmentation fault.
> 
> In the case of glibc, this is not an issue, as per the documentation[3]
> they consider it an error:
> 
>        EINVAL locale is NULL.
> 
> Unfortunately, this is a difference in the implementation between glibc
> and musl, maybe due to the fact that the standard[4] in not clear in
> this point:
> 
> 
> The newlocale() function may fail if:
> 
> [EINVAL]
>     The locale argument is not a valid string pointer. 

This is specifically documented as a "may fail", not a "shall fail",
i.e. it's not guaranteed to happen. It comes from POSIX, and is an
instance of a weird pattern the committee tried to fix (but missed
some places it seems) where they wrote "may fail"s for conditions that
already have undefined behavior (here, use of an invalid pointer) in
which case EINVAL would already be allowed as a side effect of the UB
without any further specification. (The same thing created a lot of
confusion in the past about use of pthread_t values past the end of
their lifetime.)

> My personal believe is that adding a NULL pointer check in musl is very
> simple and might help not only GNOME desktop, but maybe also other
> projects in the future. This is the reason why I brought the issue here
> first instead of directly patching GNOME desktop. If you believe that
> musl behaviour should remain the way it is, please let me know and I
> will send MRs for upstream and Alpine's GNOME desktop. I am not
> subscribed to the mailing list, so I would appreciate if I am CC'ed in
> any response.

The musl behavior should remain the way it is. My text on the
rationale actually made it into the glibc wiki some years back:

https://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers

    "The GNU C library considers it a QoI feature not to mask user
    bugs by detecting invalid pointers and returning EINVAL (unless
    the API is standardized and says it does that). If passing a bad
    pointer has undefined behavior, it is far more useful in the long
    run if it crashes quickly rather than diagnosing an error that is
    probably ignored by the flaky caller.

    If you're going to check for NULL pointer arguments where you have
    not entered into a contract to accept and interpret them, do so
    with an assert, not a conditional error return. This way the bugs
    in the caller will be immediately detected and can be fixed, and
    it makes it easy to disable the overhead in production builds. The
    assert can be valuable as code documentation. However, a segfault
    from dereferencing the NULL pointer is just as effective for
    debugging. If you return an error code to a caller which has
    already proven itself buggy, the most likely result is that the
    caller will ignore the error, and bad things will happen much
    later down the line when the original cause of the error has
    become difficult or impossible to track down. Why is it reasonable
    to assume the caller will ignore the error you return? Because the
    caller already ignored the error return of malloc or fopen or some
    other library-specific allocation function which returned NULL to
    indicate an error."

In particular, here it seems to have found a bug -- what could the
application have possibly meant by passing a null pointer there? Did
it actually intend the behavior of ""? Or of "C"? Or if the intent was
to have this mean "don't use a context-local locale", why pass the
pointer to newlocale and process the error (which could include a
number of other errors you'd certainly want to treat differently)
rather than checking for null and taking a different code path before
calling newlocale?

Rich

  reply	other threads:[~2021-10-06 12:29 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-06  9:31 Pablo Correa Gomez
2021-10-06 12:29 ` Rich Felker [this message]
2021-10-06 12:57   ` Pablo Correa Gomez
2021-10-06 12:31 ` Quentin Rameau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211006122936.GD2559@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=ablocorrea@hotmail.com \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).