On Wed, Jul 09 2025, Pip Cet wrote:

[...]
> Thanks, LGTM.  Hope that applies to the other accumulated patches, too,
> then I'll install them all.

Thanks.

> This is a preexisting bug, but I'm suspicious about the code leaking a
> pointer to an igc_xzalloc'd area to its caller, which may call Lisp
> (when it autoloads a map) in the case of Fminor_mode_key_binding.  If
> that Lisp code recursively accesses keymaps and the maps get
> reallocated, we end up resurrecting free'd data, and will probably
> crash.  (My suspicion is this is the reason for the "don't use realloc"
> comment, too).
>
> We should really fix this on master and just use vectors, and it's
> probably even less effort for you to do so than it is for me, but I
> still don't get how code like this is meant to be fixed.
>
> (I hope the comment about static being defined to the empty string is no
> longer accurate :-) ).

:-) That's a curious comment.  The overflow check in current_minor_maps
is also curious.

A simple fix for Fminor_mode_key_binding would be to copy the modes and
maps arrays to an alloca'd area before calling Flookup_key.

In the patch below (for master) I tried something different: remove the
global variables cmm_maps and cmm_modes entirely.

Assuming that there are rarely more than a handful of minor modes
active, we can allocate the temporary arrays on the stack; for the case
with very many minor modes we use the GC heap.

The patch uses the GC heap if there are more than 16 minor mode keymaps
active.  I'm not sure if that is enough; viper mode already activates 8.

A nice aspect of the patch is that Fcurrent_active_maps no longer uses
the temparary arrays: the code in the patch knows how to build a list in
reverse order.  So with the patch, current_minor_maps is called less
frequently.  The most frequent callers are now tool_bar_items and
tab_bar_items which call current_minor_maps when the user scrolls
around.

Helmut