Division by zero crash

[mkroman]

Hi,

I have a particular rar file that causes rar2fs 1.22.0 to crash due to division by zero.

[260390.289580] traps: rar2fs[12904] trap divide error ip:40c26e sp:7f64953f7a00 error:0 in rar2fs[400000+1d000]

Backtrace:

(gdb) bt
#0  0x000000000040c26e in lread_raw (buf=0x7f014403ea20 "\310\a", size=15203, offset=826966016, fi=0x7f014badabf0) at rar2fs.c:816
#1  0x0000000000415d81 in rar2_read (path=0x0, buffer=0x7f014403ea20 "\310\a", size=16384, offset=826966016, fi=0x7f014badabf0) at rar2fs.c:3996
#2  0x00007f014cf35574 in ?? ()
#3  0x00007f014410cd60 in ?? ()
#4  0x00007f014410cd10 in ?? ()
#5  0x0000000000020000 in ?? ()
#6  0x000000000178f630 in ?? ()
#7  0x00007f0144116c30 in ?? ()
#8  0x0000000000003d0b in ?? ()
#9  0x0000000000004000 in ?? ()
#10 0x00000000314a8000 in ?? ()
#11 0x00007f014badabf0 in ?? ()
#12 0x00007f014cf35732 in ?? ()
#13 0x00000000314abb63 in ?? ()
#14 0x0000000000000000 in ?? ()

The offending line (note the line difference is due to difference in version)

(gdb) print (offset - op->entry_p->vsize_first)
$8 = 776966109
(gdb) print (offset - op->entry_p->vsize_first) / op->entry_p->vsize_next 
Division by zero
(gdb) print op->entry_p->vsize_next 
$9 = 0

My naive attempt at fixing the problem:

vol = offset < VOL_FIRST_SZ ? 0 :
        1 + (VOL_NEXT_SZ > 0 ? ((offset - VOL_FIRST_SZ) / VOL_NEXT_SZ) : 0);

[hasse69]

Thanks for the report. There is not test for this because vsize_next should never become zero! Is there something special with this archive? Like it contains only 2 volumes files or are missing some files etc. Using your patch, can you try to extract the data using unrar and diff an extracted file with the file read from rar2fs to see if they are still reported the same?

[hasse69]

Can you also dump vsize_first when you see the error.

[mkroman]

The archive in question is confirmed to be corrupt - unrar complains about the CRC checksum, but since it's corrupt I'm not concerned about the contents of the archive being correct.

(gdb) print (offset - op->entry_p->vsize_first)
$1 = 776966109
(gdb) print op->entry_p->vsize_first
$2 = 49999907
(gdb) print op->entry_p->vsize_next
$3 = 0
(gdb) print offset
$4 = 826966016

My first fix did not make much sense and it still crashed, so I just added a check and return EOF if vsize_next is 0. It still hasn't crashed.

--- ../old/rar2fs-1.22.0/rar2fs.c   2016-01-08 12:15:23.000000000 +0100
+++ rar2fs.c    2016-07-03 02:21:24.882559189 +0200
@@ -812,6 +812,15 @@
                                 }
                         }

+                        /*
+                         * If we're trying to read past the first volume and the size of
+                         * the next volume is zero, assume something is wrong and return
+                         * EOF.
+                         */
+                        if (offset > VOL_FIRST_SZ && !VOL_NEXT_SZ) {
+                                return 0;
+                        }
+
                         vol = offset < VOL_FIRST_SZ ? 0 :
                                 1 + ((offset - VOL_FIRST_SZ) / VOL_NEXT_SZ);
                         chunk = offset < VOL_FIRST_SZ ? VOL_FIRST_SZ - offset :

[hasse69]

Right, but I am not compltely happy about the patch yet.My concern is why this was not discovered already when the cache was populated. vsize_next could only have becomes 0 in one place, and that is here

entry_p->vsize_next = GET_RAR_PACK_SZ(&next->hdr);

Because initially it is set to the same as visize_first. Can you try this patch instead of yours?

diff --git a/rar2fs.c b/rar2fs.c
index 1b637c0..0a7fb33 100644
--- a/rar2fs.c
+++ b/rar2fs.c
@@ -2336,7 +2336,8 @@ static int listrar(const char *path, struct dir_entry_list **buffer,
                 if (entry_p)  {
                         if (!entry_p->flags.vsize_resolved) {
                               entry_p->vsize_real_next = next->FileDataEnd;
-                              entry_p->vsize_next = GET_RAR_PACK_SZ(&next->hdr);
+                              entry_p->vsize_next = GET_RAR_PACK_SZ(&next->hdr) ?
+                                      GET_RAR_PACK_SZ(&next->hdr) : entry_p->vsize_next;
                               entry_p->flags.vsize_resolved = 1;
                               /*
                                * Check if we might need to compensate for the

[mkroman]

I've been running with your patch for a couple of hours now, and it still hasn't crashed. Looks good!

[hasse69]

ok, there is a root cause to the pack size of 0. It is obvious to be related to a bad archive but I would have expected it to be discovered much sooner. But on the other hand it does not really matter since this patch is still good since it protects from a division by 0 problem. The reason why the latter patch is slightly better is because you need to look only once and not every time the file is accessed.

[hasse69]

Closing this issue. File a new issue report if the problem is observed again.