From: Crystal Kolipe Subject: Re: Fix for vi editing mode in sftp(1) (PING) To: Walter Alejandro Iglesias Cc: Stuart Henderson , tech@openbsd.org Date: Mon, 19 Jan 2026 23:44:30 +0000 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.