Avoid accessing cur_flist->files at negative index #290
Conversation
… for non-regular file
|
The way C logic works, an "or" expression short circuits at the first bit that is true. Thus, if i is less than 0, the 2nd part of the or doesn't need to be (and isn't) evaluated. |
|
Given that the code you cite in the crash backtrace isn't anywhere in rsync, I looked around and it appears to be some bogus code in the copy-devices.diff of the rsync patches repo. Remember that the patches are test code, and are thus not well tested. I'll improve that patch a bit. |
This crash if from rsync from RHEL 7. |
|
Yes, I am aware of ISO C specifying short-circuit evaluation for the logical 'and', but this doesn't guarantee that the compiler won't try to calculate the second operand even if the first one is false - due to optimizations, this calculation can be done in advance, and the trick is that this doesn't violate the standard if that calculation result is not used then. I like the explanation given in the first answer here: https://stackoverflow.com/questions/70203837/the-short-circuit-evaluation-in-c-is-not-reflected-by-compiler |
… when checking for non-regular file
Recently I've got a sigsegv in rsync with the following backtrace:
`Program terminated with signal 11, Segmentation fault.
#0 read_ndx_and_attrs (f_in=f_in@entry=3, f_out=f_out@entry=3, iflag_ptr=iflag_ptr@entry=0x7ffef8f60488, type_ptr=type_ptr@entry=0x7ffef8f60487 "\200\b", buf=buf@entry=0x7ffef8f61520 "", len_ptr=len_ptr@entry=0x7ffef8f6048c)
at rsync.c:408
408 struct file_struct *file = cur_flist->files[i];
(gdb) bt
#0 read_ndx_and_attrs (f_in=f_in@entry=3, f_out=f_out@entry=3, iflag_ptr=iflag_ptr@entry=0x7ffef8f60488, type_ptr=type_ptr@entry=0x7ffef8f60487 "\200\b", buf=buf@entry=0x7ffef8f61520 "", len_ptr=len_ptr@entry=0x7ffef8f6048c)
at rsync.c:408
#1 0x00005581443857b5 in send_files (f_in=f_in@entry=3, f_out=f_out@entry=3) at sender.c:218
#2 0x000055814438f621 in client_run (f_in=3, f_out=3, pid=pid@entry=-1, argc=argc@entry=1, argv=argv@entry=0x5581446cb240) at main.c:1167
#3 0x00005581443b35da in start_socket_client (host=, remote_argc=remote_argc@entry=1, remote_argv=remote_argv@entry=0x5581446cb248, argc=argc@entry=1, argv=argv@entry=0x5581446cb240) at clientserver.c:133
#4 0x0000558144372404 in start_client (argv=0x5581446cb240, argc=1) at main.c:1380
#5 main (argc=2, argv=0x5581446cb240) at main.c:1757
(gdb) p i
$2 = -1
(gdb) p ndx
$3 = 636051
(gdb) p cur_flist->ndx_start;
Invalid character ';' in expression.
(gdb) p cur_flist->ndx_start
$4 = 636052
(gdb)
`
It doesn't look like a good idea to check for 'i < 0' and then for cur_flist->files[i] in the same statement. I guess in most cases cur_flist->files[-1] is accesible by the process, just contains some garbage that leads to a crash, and I was unlucky.