panic on s///gre with tainted utf8 strings

[ntyni]

This is a bug report for perl from ntyni@debian.org,
generated with the help of perlbug 1.42 running under perl 5.35.10.

---

As reported by Kacper Gutowski in https://bugs.debian.org/1006280
these warn or panic in taint mode (but not without):

$ perl -Twe '$_ = $^X =~ s/./"\x{10469}"/gre'
Malformed UTF-8 character (unexpected end of string) in substitution iterator at -e line 1.
$ perl -Twe '$_ = $^X =~ s/.*/"\x{10469}"/gre'
panic: sv_pos_b2u: bad byte offset, blen=4, byte=13 at -e line 1.
$ perl -Twe '$_ = "\x{105}$^X" =~ s/./""/gre'
panic: sv_pos_b2u: bad byte offset, blen=0, byte=2 at -e line 1.

Tested to happen on Debian perls back to 5.20 or so, still happens on
5.34.0 and current blead.

#13948 seems similar but without /r at least.

-----------------------------------------------------------------
---
Flags:
    category=core
    severity=low
---
Site configuration information for perl 5.35.10:

Configured by ntyni at Tue Mar  1 18:01:49 GMT 2022.

Summary of my perl5 (revision 5 version 35 subversion 10) configuration:
  Commit id: f0ef35deb0072805b68cef82ebe28739f67ca79a
  Platform:
    osname=linux
    osvers=5.14.0-2-amd64
    archname=x86_64-linux
    uname='linux carme 5.14.0-2-amd64 #1 smp debian 5.14.9-2 (2021-10-03) x86_64 gnulinux '
    config_args='-des -Dusedevel'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='cc'
    ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
    optimize='-O2'
    cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='11.2.0'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='cc'
    ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib
    libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.32.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.32'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'


---
@INC for perl 5.35.10:
    lib
    /usr/local/lib/perl5/site_perl/5.35.10/x86_64-linux
    /usr/local/lib/perl5/site_perl/5.35.10
    /usr/local/lib/perl5/5.35.10/x86_64-linux
    /usr/local/lib/perl5/5.35.10

---
Environment for perl 5.35.10:
    HOME=/home/ntyni
    LANG=C
    LANGUAGE (unset)
    LC_CTYPE=fi_FI.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/games
    PERL_BADLANG (unset)
    SHELL=/bin/zsh

[hvds]

Thanks, bisect shows all 3 cases start to show those diagnostics from 25fdce4 "Stop pos() from being confused by changing utf8ness".

[jkeenan]

This is a bug report for perl from ntyni@debian.org, generated with the help of perlbug 1.42 running under perl 5.35.10.

As reported by Kacper Gutowski in https://bugs.debian.org/1006280 these warn or panic in taint mode (but not without):

$ perl -Twe '$_ = $^X =~ s/./"\x{10469}"/gre'
Malformed UTF-8 character (unexpected end of string) in substitution iterator at -e line 1.
$ perl -Twe '$_ = $^X =~ s/.*/"\x{10469}"/gre'
panic: sv_pos_b2u: bad byte offset, blen=4, byte=13 at -e line 1.
$ perl -Twe '$_ = "\x{105}$^X" =~ s/./""/gre'
panic: sv_pos_b2u: bad byte offset, blen=0, byte=2 at -e line 1.

Tested to happen on Debian perls back to 5.20 or so, still happens on 5.34.0 and current blead.

#13948 seems similar but without /r at least.

Confirmed on FreeBSD-12 (perl-5.32.1, threaded build) with different value for byte in the second invocation.

$ perl -Twe '$_ = $^X =~ s/./"\x{10469}"/gre'
Malformed UTF-8 character (unexpected end of string) in substitution iterator at -e line 1.
$ perl -we '$_ = $^X =~ s/./"\x{10469}"/gre'

$ perl -Twe '$_ = $^X =~ s/.*/"\x{10469}"/gre'
panic: sv_pos_b2u: bad byte offset, blen=4, byte=19 at -e line 1.
$ perl -we '$_ = $^X =~ s/.*/"\x{10469}"/gre'

$ perl -Twe '$_ = "\x{105}$^X" =~ s/./""/gre'
panic: sv_pos_b2u: bad byte offset, blen=0, byte=2 at -e line 1.
$ perl -we '$_ = "\x{105}$^X" =~ s/./""/gre'
$ 

[jkeenan]

The warning and the two panics appear to have been introduced in the same commit cited in #13948 (comment) but not cleared up cda67c9.

On FreeBSD-12, threaded builds:

$ ./perl -v |head -2 | tail -1
This is perl 5, version 19, subversion 4 (v5.19.4 (v5.19.3-110-g428ccf1e2d)) built for amd64-freebsd-thread-multi
$ ./perl -Twe '$_ = $^X =~ s/./"\x{10469}"/gre'
$ ./perl -Twe '$_ = $^X =~ s/.*/"\x{10469}"/gre'
$ ./perl -Twe '$_ = "\x{105}$^X" =~ s/./""/gre'
$ 

$ ./perl -v |head -2 | tail -1
This is perl 5, version 19, subversion 4 (v5.19.4 (v5.19.3-111-g25fdce4a16)) built for amd64-freebsd-thread-multi
$ ./perl -Twe '$_ = $^X =~ s/./"\x{10469}"/gre'
Malformed UTF-8 character (unexpected end of string) in substitution iterator at -e line 1.
# many repetitions of above warning

$ ./perl -Twe '$_ = $^X =~ s/.*/"\x{10469}"/gre'
panic: sv_pos_b2u: bad byte offset, blen=4, byte=35 at -e line 1.

$ ./perl -Twe '$_ = "\x{105}$^X" =~ s/./""/gre'
panic: sv_pos_b2u: bad byte offset, blen=0, byte=2 at -e line 1.

@iabyn, Dave, can you take a look?

Thank you very much.
Jim Keenan

[demerphq]

I this @iabyn or is it @khwilliamson ?

[khwilliamson]

I've never dealt with this code, but I single stepped through it, and see how it is going astray, but I don't know what the code should be doing, given the lack of comments.

The failure is happening in Perl_sv_pos_b2u_flags(pTHX_ SV *const sv, STRLEN const offset, U32 flags)

After the first character has been substituted, sv contains it, and it has 4 bytes, which is how big U+10469 is. We are now about to put the second character in, so 'offset' is one. blen gets set to 4; presumably that stands for byte length, and that is the correct value.

But in line 8021 send = s + offset;
we constrain the string to just one byte.
We drop down, skipping most of the intermediate code, and get to 8072
if (!found || PL_utf8cache < 0) {
const STRLEN real_len = utf8_length(s, send);
It's calling utf8_length and passing in send as if it were only a single byte string. utf8_length knows this isn't legal, and raises the warning, while returning an error code of 0, which isn't checked for, so it goes blindly on.

This function doesn't get called at all without taint turned on. So it looks like tainting turns on a particular magic which is actually irrelevant to this operation, but because magic is on, it tries this, probably because the magic could include this type (but doesn't)

I'm hoping someone knows how this logic is supposed to proceed, and can take it from here, or give me hints.

[hvds]

@khwilliamson Just taking a look at this, my first question is why a random call to sv_magic() (in this case to mark as tainted) needs to reach code to "force pos to be stored as characters, not bytes". Surely this check should happen either earlier (when it first qualifies as SvMAGICAL(sv) && DO_UTF8(sv)) or later (eg on mg_find)?

This block was introduced in 25fdce4, but reading through that commit does not make the intent clear to me. Removing it causes no test failures (on an unthreaded build with debugging), and allows @ntyni's three examples to complete without complaint.

If we get no other ideas, I'd be tempted to apply the below after 5.36 is released to see if there is any fallout.

--- a/sv.c
+++ b/sv.c
@@ -5724,16 +5724,6 @@ Perl_sv_magic(pTHX_ SV *const sv, SV *const obj, const int how,
         }
     }
 
-    /* Force pos to be stored as characters, not bytes. */
-    if (SvMAGICAL(sv) && DO_UTF8(sv)
-      && (mg = mg_find(sv, PERL_MAGIC_regex_global))
-      && mg->mg_len != -1
-      && mg->mg_flags & MGf_BYTES) {
-        mg->mg_len = (SSize_t)sv_pos_b2u_flags(sv, (STRLEN)mg->mg_len,
-                                               SV_CONST_RETURN);
-        mg->mg_flags &= ~MGf_BYTES;
-    }
-
     /* Rest of work is done else where */
     mg = sv_magicext(sv,obj,how,vtable,name,namlen);
 

[khwilliamson]

@hvds analysis seems correct to me, and I concur.

[hvds]

If we get no other ideas, I'd be tempted to apply the below after 5.36 is released to see if there is any fallout.

PR now available as #19820.

[hvds]

#19820 is merged as an experimental fix for this; leaving the ticket open for now to see what fallout me might get.

[tonycoz]

Is this fix still considered experimental? It has been proposed for inclusion in 5.36.1.

[hvds]

Is this fix still considered experimental? It has been proposed for inclusion in 5.36.1.

As far as I'm aware there was no fallout from it since last June, so I'm certainly happy for it to be in 5.38. To consider for 5.36.1, it would be useful to get a second opinion, eg from @iabyn or @demerphq.

Absent negative thoughts from either of them, this should in any case be considered no longer experimental. So this ticket can be closed once they've had a chance to comment.