Possible Segfault in strip_sgr

[brodieG]

See r-lib/pillar#123.

> fansi::strip_sgr(c("\033[m", "\033[m"))

 *** caught segfault ***
address 0x1, cause 'memory not mapped'

Traceback:
 1: .Call(FANSI_strip_csi, enc2utf8(x), strip.int, warn)
 2: fansi::strip_sgr(c("\033[m", "\033[m"))

Failed to reproduce locally, including running with valgrind on R devel from March.

[brodieG]

@kazumits are you able to run your minimal example with R -d "valgrind --track-origins=yes" on a Level 2 valgrind instrumented R build on your system (see WRE). For example, I do this with Winston Chang's docker containers, although obviously this doesn't help here if the problem is specific to the compilation / R build you are using. I was not able to reproduce the error with that docker container.

You can even try a simple valgrind run with your normal R build to see if it provides a bit more information.

I haven't yet peered at the code to see if I can detect something obviously wrong, but without being able to reproduce the crash it will be much harder for me to debug. Any additional information about the location of the problem in the C code will help.

[krlmlr]

Thanks a lot for looking into it!

[kazumits]

@brodieG thanks for your instruction but R -d valgrind failed with my R (ICC + MKL).
I used gdb instead of valgrind and I found the location of the crash in the strip.c.

Program received signal SIGSEGV, Segmentation fault.
FANSI_strip (x=0x1bed628, what=0x1bed653, warn=0x1bed650) at strip.c:169
169           *res_track = '\0';

The segfault was gone when I deleted the line (I'm not sure this line is necessary)
and I also found that the segfault occurred when compiling fansi with icc -O2 or -O3.
(icc -O0 and -O1 had no problem with this.)

I hope the information will help.

[krlmlr]

I agree that setting the null terminator shouldn't be necessary because later mkCharLenCE() is used to allocate the string. But the buffer should be large enough, provided that LENGTH() on a CHARSXP really returns the number of bytes in the buffer.?

[brodieG]

Thanks, this is great. I'll look at it in detail in the next couple of days. Based on @krlmr's comment, and the fact that it works fine with low optimizations but segfaults in high optimizations almost makes me wonder if something is going wrong with the R ICC + MKL compilation (although 100% of the times in the past when I've blamed the compiler, I've been wrong).

[kazumits]

I've narrowed down the reproducible condition.

Compiling fansi with icc -O2 was sufficient (using any compiler/version of R),
and then has_ansi flag in strip.c was unintentionally "optimized out" by the compiler.

It may cause res_track to be in an unallocated memory space.

[brodieG]

Excellent, thanks for putting in the hard work here. I'll be working on this package this week so I'll try to write this in a way that always compiles correctly.

[brodieG]

I believe this is fixed in the development branch. I was about ready to blame ICC, but I think that other compilations were benefiting from undefined behavior that allowed them to work properly where the re-declaration of a variable in a loop was not overwriting the previous pointer location.

@kazumits unfortunately I do not have an ICC license; would you be able to test the development branch to see if the error persists?

[krlmlr]

What a subtle issue, thanks again for looking into it!

[kazumits]

This error has been completely gone in the development branch using icc -O2 and -O3.

> fansi::strip_sgr(c("\033[m","\033[m"))
[1] "" ""
> pillar::squeeze(list(1:2))
  <int>
1     1
2     2

I do appreciate your efforts.