Shell-quoting garbled after parsing UTF-8 characters about ksh HOT 9 CLOSED

I've localised the problem to this part of sh_fmtq():

If we insert the following debug command on line 373 and recompile:

errormsg(SH_DICT,ERROR_warn(0),"[DEBUG] length == %d", cp - string - 1);

then the output of the test script above becomes:

test.ksh[2]: printf: warning: [DEBUG] length == 25
'Is shell-quoting garbled?'
test.ksh[4]: printf: warning: [DEBUG] length == 26
'Is shell-quoting garbled?

Note that the garbled string has its length counted wrong: one character more, not less. This means that stakwrite(string,c); on line 376 writes one byte too many, which is the C string's terminating zero. This explains why the final quote doesn't appear; it is dutifully added, but after the erroneous zero.

(later edit: if a longer UTF-8 character is used, e.g. the four-byte '🇪🇺', then the wrong length in the second debug output correspondingly increases from 26 to 28 -- three bytes too many.)

So there is some problem with multibyte character processing. My working hypothesis is that the read -n1 g command in the test script, which triggers the bug and which reads one byte (not one character), leaves some kind of inconsistent multibyte character processing state, as reading one byte is reading half of that multibyte character.

First suspect is the mbchar(cp) call on line 360. Evidently this returns the current character and moves the character pointer (cp) to the next character in a multibyte-compatible way.

mbchar() looks like a function, but is a macro. This macro and its friends are defined here:

To put this to the test, insert ast.mb_cur_max = 0 on line 337 in string.c - right at the beginning of the sh_fmtq() function. Sure enough, after doing that and recompiling, the bug vanishes.

This is a hack, though, not a proper fix. The real bug is in the multibyte macros, not in the shellquoting algorithm. More digging to follow…

from ksh.

One of those ast.h macros that is catching my attention is the one on line 190:

#define mbinit()	(mbwide()?(*ast.mb_towc)((wchar_t*)0,(char*)0,mbmax()):0) 

This macro's name suggests that is is meant to initialise the state of multibyte character processing. It is called in various places where multibyte characters are processed, but not in sh_fmtq().

However, unfortunately, putting a mbinit() instead of ast.mb_sync = 0 on line 337 in sh/string.c makes the bug reappear. But what good is that whole mbinit() thing then?

If we're currently in a multibyte locale (i.e.: mbwide() is true a.k.a. ast.mb_cur_max > 1), then mbinit() calls the function pointed to by ast.mb_towc with the first two parameters being null pointers and the third the current value of ast.mb_cur_max. But what function is it calling?

The answer is in the set_ctype discipline function quoted above, on line 2244 to be precise: in a UTF-8 locale, ast.mb_towc = utf8_mbtowc. (Not sure what wc is an acronym of... wordcode? wide characters?) Anyway, here is that utf8_mbtowc() function:

Which means that, in UTF-8 locales, invoking mbinit() does nothing. It's a no-op.

Maybe this is the actual bug. This init routine should be resetting ast.mb_sync to zero, as set_ctype() does. If ast.mb_sync is erroneously non-zero, then you can sort of see in the mbchar() macro definition (which I still find very hard to understand) that too much gets added to the length. This is starting to make some kind of sense now.

from ksh.

Understood. Thank you for all the hard work on ksh.

from ksh.

My line 337 ast.mb_cur_max = 0 hack in the previous message is wrong. It introduces another bug:

$ cat > test2.ksh
LANG=C.UTF-8
var=múltîbytè
printf 'length of var: %d\n' ${#var}
printf '%q\n' 'Is shell-quoting garbled?'
printf 'length of var: %d\n' ${#var}
print -nr $'\303\274' | read -n1 g
printf '%q\n' 'Is shell-quoting garbled?'
$ arch/*/bin/ksh test2.ksh
length of var: 9
'Is shell-quoting garbled?'
length of var: 12
'Is shell-quoting garbled?'

Of course the length of the variable should be 9 in both cases. This means that ast.mb_cur_max does not store any kind of temporary state and that setting it to zero actually disables multibyte processing altogether, which of course masks the shellquoting bug.

Grepping the code to find where ast.mb_cur_max is set points us to this set_ctype() function:

$ arch/*/bin/ksh test2.ksh
length of var: 9
'Is shell-quoting garbled?'
length of var: 9
'Is shell-quoting garbled?'

This also makes some sense of how those mb* macros are defined, if you read them carefully (good luck).

So this may be a better hack, but it's still a hack. Stay tuned.

from ksh.

So, it looks like the following patch makes the bug reliably go away without causing regressions when running bin/shtests --utf8:

diff --git a/src/lib/libast/comp/setlocale.c b/src/lib/libast/comp/setlocale.c
index ba31fe9..65553f8 100644
--- a/src/lib/libast/comp/setlocale.c
+++ b/src/lib/libast/comp/setlocale.c
@@ -600,6 +600,8 @@ utf8_mbtowc(wchar_t* wp, const char* str, size_t n)
        register int            c;
        register wchar_t        w = 0;

+       if (!wp && !sp)
+               ast.mb_sync = 0;  /* assume call from mbinit() macro: reset global multibyte sync state */
        if (!sp || !n)
                return 0;
        if ((m = utf8tab[*sp]) > 0)

from ksh.

With the patch in the previous comment, it looks like sh_fmtq() doesn't even need a new mbinit() call/expansion for the bug to stay gone. So, apparently, this patch fixes one or more problems elsewhere in the code that left the global multibyte sync state inconsistent due to no-op mbinit() calls, which is a good sign.

I think I'll add that mbinit() call to sh_fmtq() anwyay, though: it will probably make it more bug-proof even if I can't trigger any bugs now. (Compare sh_substitute() in string.c lines 190-192, which does the same.)

from ksh.

FYI: I do not experience the missing closing single quote using ksh93v-.

$ echo $KSH_VERSION
Version ABIJM 93v- 2014-12-24
$ LANG=C.UTF-8
$ printf '%q\n' 'Is shell-quoting garbled?'
'Is shell-quoting garbled?'
$ print -nr $'\303\274' | read -n1 g
$ printf '%q\n' 'Is shell-quoting garbled?'
'Is shell-quoting garbled?'
$

from ksh.

Thanks @hyenias. The 93v- beta completely overhauled the whole multibyte processing system in a new version of the libast API, so it wasn't possible to backport a fix, nor was it feasible to backport that entire overhauled system as it's tied in with lots of other changes in the code.

from ksh.

Just to add to the record, I'd forgotten I'd found another trigger of this bug back in 2017: att#13 (comment)

$ ksh -c 'i=$IFS; IFS=é; set : :; echo "$*"; IFS=$i; trap "echo end" EXIT; trap'
:?:
trap -- 'echo end EXIT
end

Note the missing quote after trap -- 'echo end. You get the same with the output of export -p, alias, etc.

I can happily confirm that this was fixed by 300cd19 as well (but of course IFS still doesn't actually handle multibyte characters).

from ksh.