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

From: Aurelien Jarno <[email-masked]>
Date: Tue, 5 Nov 2024 23:45:20 +0100

control: severity -1 important
control: reassign -1 gcc-14
control: retitle -1 gcc: riscv64 backend emits large relocations due to loop strength reduction
control: forwarded -1 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117460
control: affects -1 lziprecover

On 2024-11-05 21:47, Jessica Clarke wrote:
> 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?

We continued the discussion on IRC, and Jessica pointed me that's
actually a GCC bug that I have now reported upstream. I am therefore
retitling and reassigning this bug accordingly.

Regards
Aurelien 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email-masked]                     http://aurel32.net