Subject: Bug#1086791: lziprecover: FTBFS on riscv64 due to out-of-bounds access on a constant

From: Jessica Clarke <[email-masked]>
Date: Tue, 5 Nov 2024 21:47:24 +0000

On 5 Nov 2024, at 21:27, Aurelien Jarno <[email-masked]> wrote:
> 
> Source: lziprecover
> Version: 1.25~pre1-1
> Severity: serious
> Tags: ftbfs upstream
> Justification: fails to build from source (but built successfully in the past)
> X-Debbugs-Cc: [email-masked]
> User: [email-masked]
> Usertags: riscv64
> 
> Dear maintainer,
> 
> lziprecover fails to build on riscv64 with the following error:
> 
> | riscv64-linux-gnu-g++ -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -ffile-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -DPROGVERSION=\"1.25-pre1\" -c -o main.o main.cc
> | riscv64-linux-gnu-g++ -g -O2 -ffile-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wl,-z,relro -o lziprecover arg_parser.o alone_to_lz.o lzip_index.o list.o byte_repair.o dump_remove.o fec_create.o fec_repair.o gf8.o gf16.o lunzcrash.o md5.o merge.o mtester.o nrep_stats.o range_dec.o recursive.o reproduce.o split.o decoder.o main.o -lpthread
> | main.o: in function `(anonymous namespace)::set_mode((anonymous namespace)::Mode&, (anonymous namespace)::Mode) [clone .part.0]':
> | /usr/include/riscv64-linux-gnu/bits/stdio2.h:111:(.text+0x8b0): relocation truncated to fit: R_RISCV_PCREL_HI20 against `.LC15'
> | collect2: error: ld returned 1 exit status
> | make[1]: *** [Makefile:49: lziprecover] Error 1
> | make[1]: Leaving directory '/<<PKGBUILDDIR>>'
> | dh_auto_build: error: make -j4 returned exit code 2
> | make: *** [debian/rules:6: binary-arch] Error 25
> | dpkg-buildpackage: error: debian/rules binary-arch subprocess returned exit status 2
> | --------------------------------------------------------------------------------
> 
> The full build log is available there:
> https://buildd.debian.org/status/fetch.php?pkg=lziprecover&arch=riscv64&ver=1.25%7Epre1-1&stamp=1728351091&raw=0
> 
> After investigation, I have found that this problem occurs when the
> compiler inlines the newly added compare_prefix() function. This causes
> an out-of-bound access on a constant. This function is, for instance,
> called in parse_fec() that way:
> 
> |   else if( compare_prefix( arg, "repair" ) )
> |       set_mode( program_mode, m_fec_repair );
> 
> With the default arguments like in the above calls, compare_prefix() can be
> simplified as:
> 
> | // return true if arg is a non-empty prefix of target
> | bool compare_prefix( const char * const arg, const char * const target)
> |   {   
> |   if( arg[0] == target[0] )
> |     for( int i = 1; i < INT_MAX; ++i )
> |       {                         
> |       if( arg[i] == 0 ) return true;
> |       if( arg[i] != target[i] ) break;
> |       }
> |   return false;
> |   }
> 
> As you can see if the length of the arg string is longer than the target
> string, the latter is accessed out-of-bounds, possibly up to INT_MAX.

That’s not true. For the next iteration of the loop to occur, arg[i] !=
0 and arg[i] == target[i], therefore target[i] != 0 and thus the end of
target has not been reached yet. Or, put another way, if we ever reach
the end of target, either arg[i] == target[i] == 0 and so we return
true, or arg[i] != target[i] == 0 and so we break out of the loop (and
return false). This looks completely fine from that perspective to me,
with one exception, if arg and target are both the empty string then we
walk off the end (there should be a check for arg[0] != 0 in the outer
if). But presumably one argument is always a string literal rather than
external input so is known to not be empty. Or so we hope.

The error above is that there’s a signed 32-bit PC-relative relocation
whose value is truncated. That is, the binary is too big for the code
model in use. Perhaps what you’re noticing is that previously it was
just fine, but inlining has increased the code size and pushed it past
the limit. Does the binary embed large arrays of data?

Jess

> Probably INT_MAX should be replaced by "strlen(target) + 1”.