sqlite memory allocation and async signal safety

sqlite memory allocation and async signal safety

[Po Lu]

SQLite allocates various pieces of memory inside most of its functions.
This is done through the C library malloc, which immediately raises the
reentrancy question: shouldn't input be blocked around such functions?

Secondly, SQLite returns an error code upon running out of memory in an
irrecoverable fashion.  Shouldn't the code that currently signals an
ordinary error in such situations call `memory_full' instead?

Alternatively, we could make SQLite call xmalloc instead of C library
malloc, but I suppose the SQLite library is better at coping with
allocation failures of large sizes than xmalloc is, so that's probably
not the best solution.

Ideas?

Re: sqlite memory allocation and async signal safety

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Date: Thu, 24 Mar 2022 17:06:28 +0800
> 
> SQLite allocates various pieces of memory inside most of its functions.
> This is done through the C library malloc, which immediately raises the
> reentrancy question: shouldn't input be blocked around such functions?

I think we used to block input around function calls that could
allocate memory because our signal handlers, and in particular SIGIO
handler, did non-trivial stuff.  Nowadays our signal handlers just set
a flag and return, so I'm not sure this is needed anymore.  Especially
when system library malloc is called, which AFAIU is mostly async-safe
nowadays on modern platforms.

Am I missing something?

> Secondly, SQLite returns an error code upon running out of memory in an
> irrecoverable fashion.  Shouldn't the code that currently signals an
> ordinary error in such situations call `memory_full' instead?

memory_full means basically that the user should save and exit ASAP.
Is that indeed required when SQLite runs out of memory?  Isn't it
enough to just fail the SQLite-related function?

> Alternatively, we could make SQLite call xmalloc instead of C library
> malloc, but I suppose the SQLite library is better at coping with
> allocation failures of large sizes than xmalloc is, so that's probably
> not the best solution.

I see no reason to go to our malloc as long as SQLite is not supposed
to be invoked before dumping Emacs.

I'd like to hear others about these issues, though, as I'm not enough
of an expert on this stuff.

Re: sqlite memory allocation and async signal safety

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> I think we used to block input around function calls that could
> allocate memory because our signal handlers, and in particular SIGIO
> handler, did non-trivial stuff.  Nowadays our signal handlers just set
> a flag and return, so I'm not sure this is needed anymore.  Especially
> when system library malloc is called, which AFAIU is mostly async-safe
> nowadays on modern platforms.
>
> Am I missing something?

Unfortunately, most system malloc implementations are still not
async-signal safe, but if all that happens is a flag being set, then I
don't think calling block_input is required anymore.

Which flag is that, and where is it tested?

> memory_full means basically that the user should save and exit ASAP.
> Is that indeed required when SQLite runs out of memory?  Isn't it
> enough to just fail the SQLite-related function?

That's why I asked the second question: SQLite is supposedly very good
at coping with allocation failures, so when the memory allocation fails,
it probably means there is really no memory left.

> I see no reason to go to our malloc as long as SQLite is not supposed
> to be invoked before dumping Emacs.
>
> I'd like to hear others about these issues, though, as I'm not enough
> of an expert on this stuff.

Indeed, thanks for clarifying.

Re: sqlite memory allocation and async signal safety

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 24 Mar 2022 18:21:29 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think we used to block input around function calls that could
> > allocate memory because our signal handlers, and in particular SIGIO
> > handler, did non-trivial stuff.  Nowadays our signal handlers just set
> > a flag and return, so I'm not sure this is needed anymore.  Especially
> > when system library malloc is called, which AFAIU is mostly async-safe
> > nowadays on modern platforms.
> >
> > Am I missing something?
> 
> Unfortunately, most system malloc implementations are still not
> async-signal safe, but if all that happens is a flag being set, then I
> don't think calling block_input is required anymore.
> 
> Which flag is that, and where is it tested?

It depends on the signal.  Look at the signal handlers we install.
For SIGIO, this is the handler:

  void
  handle_input_available_signal (int sig)
  {
    pending_signals = true;

    if (input_available_clear_time)
      *input_available_clear_time = make_timespec (0, 0);
  }

and the flag is pending_signals.

Re: sqlite memory allocation and async signal safety

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> It depends on the signal.  Look at the signal handlers we install.
> For SIGIO, this is the handler:
>
>   void
>   handle_input_available_signal (int sig)
>   {
>     pending_signals = true;
>
>     if (input_available_clear_time)
>       *input_available_clear_time = make_timespec (0, 0);
>   }
>
> and the flag is pending_signals.

Thanks.  From what I can tell, it's checked in maybe_quit (and some
other places such as unblock_input), and it shouldn't require input to
be blocked around calls to sqlite functions.

The other signal handlers seem fine too.

Re: sqlite memory allocation and async signal safety

[Stefan Monnier]

> Unfortunately, most system malloc implementations are still not
> async-signal safe,

Since we don't do any malloc inside our signal handler, this is not
an issue.  What Eli meant I think by "async safe" is in terms of
multithreading with pthreads.


        Stefan

Re: sqlite memory allocation and async signal safety

[Andreas Schwab]

On Mär 24 2022, Eli Zaretskii wrote:

> when system library malloc is called, which AFAIU is mostly async-safe
> nowadays on modern platforms.

malloc is definitely not async-safe.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: sqlite memory allocation and async signal safety

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Po Lu <luangruo@yahoo.com>,  emacs-devel@gnu.org
> Date: Thu, 24 Mar 2022 11:21:41 +0100
> 
> On Mär 24 2022, Eli Zaretskii wrote:
> 
> > when system library malloc is called, which AFAIU is mostly async-safe
> > nowadays on modern platforms.
> 
> malloc is definitely not async-safe.

OK, but still: we no longer call malloc in our signal handlers, I
think.

Re: sqlite memory allocation and async signal safety

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> I think we used to block input around function calls that could
> allocate memory because our signal handlers, and in particular SIGIO
> handler, did non-trivial stuff.  Nowadays our signal handlers just set
> a flag and return, so I'm not sure this is needed anymore.  Especially
> when system library malloc is called, which AFAIU is mostly async-safe
> nowadays on modern platforms.
>
> Am I missing something?

Unfortunately, most system malloc implementations are still not
async-signal safe, but if all that happens is a flag being set, then I
don't think calling block_input is required anymore.

Which flag is that, and where is it tested?

> memory_full means basically that the user should save and exit ASAP.
> Is that indeed required when SQLite runs out of memory?  Isn't it
> enough to just fail the SQLite-related function?

That's why I asked the second question: SQLite is supposedly very good
at coping with allocation failures, so when the memory allocation fails,
it probably means there is really no memory left.

> I see no reason to go to our malloc as long as SQLite is not supposed
> to be invoked before dumping Emacs.
>
> I'd like to hear others about these issues, though, as I'm not enough
> of an expert on this stuff.

Indeed, thanks for clarifying.