On Mon, Jan 19, 2026 at 10:39:35PM +0100, Walter Alejandro Iglesias wrote:
> On Mon, Jan 19, 2026 at 08:16:28PM +0000, Stuart Henderson wrote:
> > well, &editor is obviously not set directly by that el_set.
> > 
> > if el_get() returns failure before it sets &editor (for whatever
> > reason), editor contains garbage, so dereferencing that won't do
> > anything good.
> > 
> 
> I don't follow what you're telling me here.  If your concern is the
> "editor" variable not to be initialized, we can do the following:

It wouldn't make much sense to initialise 'editor' in this way.

What Stuart recommends, to check the return value of el_get() before
dereferencing *editor is generally good programming practice, even though in
this specific case it's probably not necessary.

Let's look at the code in question.

You are adding a call to el_get(), which is expected to return a value in
'editor'.  You are passing the address of a pointer, I.E. editor is already a
pointer, and you are passing the address of that pointer to el_get().

The code for el_get() is in src/lib/libedit/el.c.

Actually the function is el_wget(), but there is a define elsewhere that sets
el_get() to point to it.

Reading the code, el_wget() will return -1 if el is NULL, which the earlier
call to el_init() in interactive_loop() should guarantee that it is not.

The switch() in el_wget will always match EL_EDITOR, as this is hard-coded in
your patch, so the eventual return value stored in 'rv' will be whatever
map_get_editor() returns.

The map_get_editor() code is in src/lib/libedit/map.c.

This will return -1 if editor is NULL.  Here in this function, editor is the
parameter you passed, so 'address of *editor'.  Since editor is a variable
allocated on the stack, it must have a valid address, the NULL check in
map_get_editor() will be false.

Note that this is _not_ the same as what Stuart said, that dereferencing
editor in your newly added code, (I.E. checking editor[0]), was a bad idea,
because:

* In your new code, you are dereferencing a pointer, (which must only be done
  to a _valid pointer_).

* In the map_get_editor() code, it's checking whether the address of 'editor'
  itself is non-NULL.  I.E. where the C compiler decided to store the pointer
  'const char *editor'.  And assuming you are using a compliant and non-buggy
  C compiler, it certainly will have stored it at a valid address.

From here, the only way that map_get_editor() could return -1 would be if
el->el_map.type was neither of MAP_EMACS or MAP_VI.

As you indirectly point out, the existing call to el_set() a few lines above
your new code _should_ ensure that.

However, there are various ways this assumption could come unstuck.

* What if somebody else modifies the code in the future, and doesn't realise
  that you are relying on the previous call to el_set() to initialise
  el->el_map.type?

* The call to el_set() subsequently calls map_set_editor(), which calls
  wcscmp() to compare the text string.  That's a library function - what if
  a bug was introduced to that at some point in the future, causing
  map_set_editor() to return -1?

So what Stuart suggested, whilst technically not absolutely necessary, is good
programming practice.