Possible `substr` bug in UTF-8 Corner Case

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Possible `substr` bug in UTF-8 Corner Case

R devel mailing list
I think there is a memory bug in `substr` that is triggered by a UTF-8 corner case: an incomplete UTF-8 byte sequence at the end of a string.  With a valgrind level 2 instrumented build of R-devel I get:

> string <- "abc\xEE"    # \xEE indicates the start of a 3 byte UTF-8 sequence
> Encoding(string) <- "UTF-8"
> substr(string, 1, 10)
==15375== Invalid read of size 1
==15375==    at 0x45B3F0: substr (character.c:286)
==15375==    by 0x45B3F0: do_substr (character.c:342)
==15375==    by 0x4CFCB9: bcEval (eval.c:6775)
==15375==    by 0x4D95AF: Rf_eval (eval.c:624)
==15375==    by 0x4DAD12: R_execClosure (eval.c:1764)
==15375==    by 0x4D9561: Rf_eval (eval.c:747)
==15375==    by 0x507008: Rf_ReplIteration (main.c:258)
==15375==    by 0x5073E7: R_ReplConsole (main.c:308)
==15375==    by 0x507494: run_Rmainloop (main.c:1082)
==15375==    by 0x41A8E6: main (Rmain.c:29)
==15375==  Address 0xb9e518d is 3,869 bytes inside a block of size 7,960 alloc'd
==15375==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15375==    by 0x51033E: GetNewPage (memory.c:888)
==15375==    by 0x511FC0: Rf_allocVector3 (memory.c:2691)
==15375==    by 0x4657AC: Rf_allocVector (Rinlinedfuns.h:577)
==15375==    by 0x4657AC: Rf_ScalarString (Rinlinedfuns.h:1007)
==15375==    by 0x4657AC: coerceToVectorList (coerce.c:892)
==15375==    by 0x4657AC: Rf_coerceVector (coerce.c:1293)
==15375==    by 0x4660EB: ascommon (coerce.c:1369)
==15375==    by 0x4667C0: do_asvector (coerce.c:1544)
==15375==    by 0x4CFCB9: bcEval (eval.c:6775)
==15375==    by 0x4D95AF: Rf_eval (eval.c:624)
==15375==    by 0x4DAD12: R_execClosure (eval.c:1764)
==15375==    by 0x515EF7: dispatchMethod (objects.c:408)
==15375==    by 0x516379: Rf_usemethod (objects.c:458)
==15375==    by 0x516694: do_usemethod (objects.c:543)
==15375==
[1] "abc<ee>"

Here is a patch for the native version of `substr` that highlights the problem and a possible fix.  Basically `substr` computes the byte width of a UTF-8 character based on the leading byte ("\xEE" here, which implies 3 bytes) and reads/writes that entire byte width irrespective of whether the string actually ends before the theoretical end of the UTF-8 "character".

Index: src/main/character.c
===================================================================
--- src/main/character.c    (revision 74482)
+++ src/main/character.c    (working copy)
@@ -283,7 +283,7 @@
    for (i = 0; i < so && str < end; i++) {
        int used = utf8clen(*str);
        if (i < sa - 1) { str += used; continue; }
-        for (j = 0; j < used; j++) *buf++ = *str++;
+        for (j = 0; j < used && str < end; j++) *buf++ = *str++;
    }
     } else if (ienc == CE_LATIN1 || ienc == CE_BYTES) {
    for (str += (sa - 1), i = sa; i <= so; i++) *buf++ = *str++;

The change above removed the valgrind error for me.  I re-built R with the change and ran "make check" which seemed to work fine. I also ran some simple checks on UTF-8 strings and things seem to work okay.

I have very limited experience making changes to R (this is my first attempt at a patch) so please take all of the above with extreme skepticism.

Apologies in advance if this turns out to be a false alarm caused by an error on my part.

Best,

Brodie.

PS: apologies also if the formatting of this e-mail is bad.  I have not figured out how to get plaintext working properly with yahoo.

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Possible `substr` bug in UTF-8 Corner Case

Tomas Kalibera
Thanks, fixed in R-devel (by checking validity of UTF-8 strings for
substr/substring).
Tomas

On 03/29/2018 03:53 AM, brodie gaslam via R-devel wrote:

> I think there is a memory bug in `substr` that is triggered by a UTF-8 corner case: an incomplete UTF-8 byte sequence at the end of a string.  With a valgrind level 2 instrumented build of R-devel I get:
>
>> string <- "abc\xEE"    # \xEE indicates the start of a 3 byte UTF-8 sequence
>> Encoding(string) <- "UTF-8"
>> substr(string, 1, 10)
> ==15375== Invalid read of size 1
> ==15375==    at 0x45B3F0: substr (character.c:286)
> ==15375==    by 0x45B3F0: do_substr (character.c:342)
> ==15375==    by 0x4CFCB9: bcEval (eval.c:6775)
> ==15375==    by 0x4D95AF: Rf_eval (eval.c:624)
> ==15375==    by 0x4DAD12: R_execClosure (eval.c:1764)
> ==15375==    by 0x4D9561: Rf_eval (eval.c:747)
> ==15375==    by 0x507008: Rf_ReplIteration (main.c:258)
> ==15375==    by 0x5073E7: R_ReplConsole (main.c:308)
> ==15375==    by 0x507494: run_Rmainloop (main.c:1082)
> ==15375==    by 0x41A8E6: main (Rmain.c:29)
> ==15375==  Address 0xb9e518d is 3,869 bytes inside a block of size 7,960 alloc'd
> ==15375==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==15375==    by 0x51033E: GetNewPage (memory.c:888)
> ==15375==    by 0x511FC0: Rf_allocVector3 (memory.c:2691)
> ==15375==    by 0x4657AC: Rf_allocVector (Rinlinedfuns.h:577)
> ==15375==    by 0x4657AC: Rf_ScalarString (Rinlinedfuns.h:1007)
> ==15375==    by 0x4657AC: coerceToVectorList (coerce.c:892)
> ==15375==    by 0x4657AC: Rf_coerceVector (coerce.c:1293)
> ==15375==    by 0x4660EB: ascommon (coerce.c:1369)
> ==15375==    by 0x4667C0: do_asvector (coerce.c:1544)
> ==15375==    by 0x4CFCB9: bcEval (eval.c:6775)
> ==15375==    by 0x4D95AF: Rf_eval (eval.c:624)
> ==15375==    by 0x4DAD12: R_execClosure (eval.c:1764)
> ==15375==    by 0x515EF7: dispatchMethod (objects.c:408)
> ==15375==    by 0x516379: Rf_usemethod (objects.c:458)
> ==15375==    by 0x516694: do_usemethod (objects.c:543)
> ==15375==
> [1] "abc<ee>"
>
> Here is a patch for the native version of `substr` that highlights the problem and a possible fix.  Basically `substr` computes the byte width of a UTF-8 character based on the leading byte ("\xEE" here, which implies 3 bytes) and reads/writes that entire byte width irrespective of whether the string actually ends before the theoretical end of the UTF-8 "character".
>
> Index: src/main/character.c
> ===================================================================
> --- src/main/character.c    (revision 74482)
> +++ src/main/character.c    (working copy)
> @@ -283,7 +283,7 @@
>      for (i = 0; i < so && str < end; i++) {
>          int used = utf8clen(*str);
>          if (i < sa - 1) { str += used; continue; }
> -        for (j = 0; j < used; j++) *buf++ = *str++;
> +        for (j = 0; j < used && str < end; j++) *buf++ = *str++;
>      }
>       } else if (ienc == CE_LATIN1 || ienc == CE_BYTES) {
>      for (str += (sa - 1), i = sa; i <= so; i++) *buf++ = *str++;
>
> The change above removed the valgrind error for me.  I re-built R with the change and ran "make check" which seemed to work fine. I also ran some simple checks on UTF-8 strings and things seem to work okay.
>
> I have very limited experience making changes to R (this is my first attempt at a patch) so please take all of the above with extreme skepticism.
>
> Apologies in advance if this turns out to be a false alarm caused by an error on my part.
>
> Best,
>
> Brodie.
>
> PS: apologies also if the formatting of this e-mail is bad.  I have not figured out how to get plaintext working properly with yahoo.
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Possible `substr` bug in UTF-8 Corner Case

R devel mailing list
Thank you for the quick response and for the quick fix (and for the rchk vagrant image I used to build and test the below!).
One thing I'll note about the fix is that it may start breaking things that used to "work".  I think it is fair to say that character count is not well defined with illegal UTF-8 sequences (and noteworthy that `nchar` does actually stop when it encounters them), but there may be a bit of code out there that relied on being able to successfully complete (albeit while potentially corrupting memory) that will now produce errors.  It may be worth highlighting this in the release notes.

Best,
Brodie.


    On Thursday, March 29, 2018, 9:11:15 AM EDT, Tomas Kalibera <[hidden email]> wrote:  
 
 Thanks, fixed in R-devel (by checking validity of UTF-8 strings for
substr/substring).
Tomas

On 03/29/2018 03:53 AM, brodie gaslam via R-devel wrote:

> I think there is a memory bug in `substr` that is triggered by a UTF-8 corner case: an incomplete UTF-8 byte sequence at the end of a string.  With a valgrind level 2 instrumented build of R-devel I get:
>
>> string <- "abc\xEE"    # \xEE indicates the start of a 3 byte UTF-8 sequence
>> Encoding(string) <- "UTF-8"
>> substr(string, 1, 10)
> ==15375== Invalid read of size 1
> ==15375==    at 0x45B3F0: substr (character.c:286)
> ==15375==    by 0x45B3F0: do_substr (character.c:342)
> ==15375==    by 0x4CFCB9: bcEval (eval.c:6775)
> ==15375==    by 0x4D95AF: Rf_eval (eval.c:624)
> ==15375==    by 0x4DAD12: R_execClosure (eval.c:1764)
> ==15375==    by 0x4D9561: Rf_eval (eval.c:747)
> ==15375==    by 0x507008: Rf_ReplIteration (main.c:258)
> ==15375==    by 0x5073E7: R_ReplConsole (main.c:308)
> ==15375==    by 0x507494: run_Rmainloop (main.c:1082)
> ==15375==    by 0x41A8E6: main (Rmain.c:29)
> ==15375==  Address 0xb9e518d is 3,869 bytes inside a block of size 7,960 alloc'd
> ==15375==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==15375==    by 0x51033E: GetNewPage (memory.c:888)
> ==15375==    by 0x511FC0: Rf_allocVector3 (memory.c:2691)
> ==15375==    by 0x4657AC: Rf_allocVector (Rinlinedfuns.h:577)
> ==15375==    by 0x4657AC: Rf_ScalarString (Rinlinedfuns.h:1007)
> ==15375==    by 0x4657AC: coerceToVectorList (coerce.c:892)
> ==15375==    by 0x4657AC: Rf_coerceVector (coerce.c:1293)
> ==15375==    by 0x4660EB: ascommon (coerce.c:1369)
> ==15375==    by 0x4667C0: do_asvector (coerce.c:1544)
> ==15375==    by 0x4CFCB9: bcEval (eval.c:6775)
> ==15375==    by 0x4D95AF: Rf_eval (eval.c:624)
> ==15375==    by 0x4DAD12: R_execClosure (eval.c:1764)
> ==15375==    by 0x515EF7: dispatchMethod (objects.c:408)
> ==15375==    by 0x516379: Rf_usemethod (objects.c:458)
> ==15375==    by 0x516694: do_usemethod (objects.c:543)
> ==15375==
> [1] "abc<ee>"
>
> Here is a patch for the native version of `substr` that highlights the problem and a possible fix.  Basically `substr` computes the byte width of a UTF-8 character based on the leading byte ("\xEE" here, which implies 3 bytes) and reads/writes that entire byte width irrespective of whether the string actually ends before the theoretical end of the UTF-8 "character".
>
> Index: src/main/character.c
> ===================================================================
> --- src/main/character.c    (revision 74482)
> +++ src/main/character.c    (working copy)
> @@ -283,7 +283,7 @@
>      for (i = 0; i < so && str < end; i++) {
>          int used = utf8clen(*str);
>          if (i < sa - 1) { str += used; continue; }
> -        for (j = 0; j < used; j++) *buf++ = *str++;
> +        for (j = 0; j < used && str < end; j++) *buf++ = *str++;
>      }
>       } else if (ienc == CE_LATIN1 || ienc == CE_BYTES) {
>      for (str += (sa - 1), i = sa; i <= so; i++) *buf++ = *str++;
>
> The change above removed the valgrind error for me.  I re-built R with the change and ran "make check" which seemed to work fine. I also ran some simple checks on UTF-8 strings and things seem to work okay.
>
> I have very limited experience making changes to R (this is my first attempt at a patch) so please take all of the above with extreme skepticism.
>
> Apologies in advance if this turns out to be a false alarm caused by an error on my part.
>
> Best,
>
> Brodie.
>
> PS: apologies also if the formatting of this e-mail is bad.  I have not figured out how to get plaintext working properly with yahoo.
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel


 
        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel