| From 46b1bceb88928e75124a63805899fb4498d571c0 Mon Sep 17 00:00:00 2001 |
| From: Hans Beckerus <hans.beckerus at gmail.com> |
| Date: Mon, 9 Aug 2021 21:08:18 +0200 |
| Subject: [PATCH] Fix crash in collect files() |
| |
| When collect_files() tries to identify first volume file there is a |
| missing check on a pointer to be valid or not. There are two identified |
| scenarios in which this can happen. |
| |
| 1) Failing to allocate memory that in current design could still produce |
| a pointer that is not NULL but still not valid. This is due to how C++ |
| 'operator new' by default works since the time exceptions were introduced. |
| In this case simply checking for NULL would not be enough. |
| |
| 2) Probably a lot more likely but still must be considered an extremely |
| rare case is that the function involved returns ERAR_EOPEN without |
| first having allocated this memory in the first place. This can only |
| happen in a very specific branch in the unrar library that has been |
| overlooked or it was introduced in a later version of it. It covers a |
| very old RAR compression format/version that had a flaw in the way |
| length of each file was calculated. It could result in that an entire |
| file fitted in one volume but still indicated in the header that it |
| continued in the next. Such archives must not be allowed to be opened |
| and hence an error is thrown. A simple test for NULL would cure that |
| since pointer is initialized to 0 and if no allocation is performed it |
| will remain 0 across the function call. |
| |
| Thus to solve both these scenarios the solution is to: |
| - check for NULL where relevant |
| - call operator new with the (std:nothrow) argument |
| |
| This patch also introduce some stability changes to collect_files() |
| and should avoid some other potential problems for obfuscated and |
| non-standard archive names. |
| |
| Resolves-issue: #167 |
| Signed-off-by: Hans Beckerus <hans.beckerus at gmail.com> |
| --- |
| src/dllext.cpp | 10 +++-- |
| src/rar2fs.c | 117 ++++++++++++++++++++++++++----------------------- |
| 2 files changed, 69 insertions(+), 58 deletions(-) |
| |
| diff --git a/src/dllext.cpp b/src/dllext.cpp |
| index df459ed..3b8c240 100644 |
| --- a/src/dllext.cpp |
| +++ b/src/dllext.cpp |
| @@ -86,7 +86,9 @@ int PASCAL RARListArchiveEx(HANDLE hArcData, RARArchiveDataEx **NN) |
| |
| if (!*NN) |
| { |
| - *NN = new RARArchiveDataEx; |
| + *NN = new (std::nothrow) RARArchiveDataEx; |
| + if (!*NN) |
| + return ERAR_NO_MEMORY; |
| } |
| N = *NN; |
| memcpy(&N->hdr, &h, sizeof(h)); |
| @@ -180,8 +182,6 @@ int PASCAL RARListArchiveEx(HANDLE hArcData, RARArchiveDataEx **NN) |
| } |
| } |
| #endif |
| - // Skip to next header |
| - return RARProcessFile(hArcData,RAR_SKIP,NULL,NULL); |
| } |
| #if RARVER_MAJOR > 4 || ( RARVER_MAJOR == 4 && RARVER_MINOR >= 20 ) |
| catch (std::bad_alloc&) // Catch 'new' exception. |
| @@ -191,9 +191,11 @@ int PASCAL RARListArchiveEx(HANDLE hArcData, RARArchiveDataEx **NN) |
| *NN = NULL; |
| } |
| cerr << "RARListArchiveEx() caught std:bac_alloc error" << endl; |
| + return ERAR_NO_MEMORY; |
| } |
| #endif |
| - return 0; |
| + // Skip to next header |
| + return RARProcessFile(hArcData,RAR_SKIP,NULL,NULL); |
| } |
| |
| void PASCAL RARFreeArchiveDataEx(RARArchiveDataEx **NN) |
| diff --git a/src/rar2fs.c b/src/rar2fs.c |
| index 7d25735..1d97d8a 100644 |
| --- a/src/rar2fs.c |
| +++ b/src/rar2fs.c |
| @@ -886,6 +886,10 @@ static int get_vformat(const char *s, int t, int *l, int *p) |
| } |
| } |
| |
| + /* Sanity check */ |
| + if (len <= 0 || pos < 0 || (size_t)(len + pos) > SLEN) |
| + return -1; |
| + |
| if (l) *l = len; |
| if (p) *p = pos; |
| return vol; |
| @@ -907,6 +911,8 @@ static int __RARVolNameToFirstName(char *arch, int vtype) |
| int ret = 0; |
| |
| vol = get_vformat(arch, !vtype, &len, &pos); |
| + if (vol == -1) |
| + return -1; |
| RARVolNameToFirstName(arch, vtype); |
| |
| memset(&header, 0, sizeof(header)); |
| @@ -1748,20 +1754,15 @@ out: |
| * Identifies all the files that are part of the same multipart archive and |
| * located in the same directory as |arch| and stores their paths. |
| * |
| - * Returns the number of files making the multipart archive. |
| - * Returns 1 if |arch| is not part of a multipart archive. |
| - * Returns a negative ERAR error code in case of error. |
| + * Returns 0 on success. |
| + * Returns a negative ERAR_ error code in case of error. |
| ****************************************************************************/ |
| static int collect_files(const char *arch) |
| { |
| RAROpenArchiveDataEx d; |
| - int files; |
| + struct RARHeaderDataEx header; |
| char *arch_; |
| - int format; |
| struct dir_entry_list *list; |
| - int vol = -1; |
| - int pos; |
| - int len; |
| |
| memset(&d, 0, sizeof(RAROpenArchiveDataEx)); |
| d.ArcName = (char *)arch; /* Horrible cast! But hey... it is the API! */ |
| @@ -1774,7 +1775,6 @@ static int collect_files(const char *arch) |
| if (!arch_) |
| return -ERAR_NO_MEMORY; |
| |
| -again: |
| h = RAROpenArchiveEx(&d); |
| |
| /* Check for fault */ |
| @@ -1785,41 +1785,47 @@ again: |
| return -d.OpenResult; |
| } |
| |
| - format = IS_NNN(arch) ? 1 : VTYPE(d.Flags); |
| - if (vol == -1) |
| - vol = get_vformat(arch, format, &len, &pos); |
| - if (d.Flags & ROADF_VOLUME && !(d.Flags & ROADF_FIRSTVOLUME)) { |
| - if (vol) { |
| - char *tmp; |
| - RARCloseArchive(h); |
| - --vol; |
| - tmp = get_vname(format, arch_, vol, len, pos); |
| + if (d.Flags & ROADF_VOLUME) { |
| + int format = IS_NNN(arch_) ? 1 : VTYPE(d.Flags); |
| + if (__RARVolNameToFirstName(arch_, !format)) { |
| free(arch_); |
| - arch_ = tmp; |
| - d.ArcName = (char *)arch_; |
| - goto again; |
| + return -ERAR_EOPEN; |
| + } |
| + RARCloseArchive(h); |
| + d.ArcName = (char *)arch_; |
| + h = RAROpenArchiveEx(&d); |
| + |
| + /* Check for fault */ |
| + if (d.OpenResult != ERAR_SUCCESS) { |
| + if (h) |
| + RARCloseArchive(h); |
| + free(arch_); |
| + return -d.OpenResult; |
| } |
| } |
| |
| RARArchiveDataEx *arc = NULL; |
| int dll_result = RARListArchiveEx(h, &arc); |
| - if (dll_result && dll_result != ERAR_EOPEN) { |
| - if (dll_result != ERAR_END_ARCHIVE) { |
| - RARFreeArchiveDataEx(&arc); |
| - RARCloseArchive(h); |
| - free(arch_); |
| - return -dll_result; |
| - } |
| + if (dll_result != ERAR_SUCCESS) { |
| + if (dll_result == ERAR_EOPEN && arc) |
| + dll_result = ERAR_SUCCESS; |
| + if (dll_result == ERAR_END_ARCHIVE && !arc) |
| + dll_result = ERAR_EOPEN; |
| + } |
| + if (dll_result != ERAR_SUCCESS && dll_result != ERAR_END_ARCHIVE) { |
| + RARFreeArchiveDataEx(&arc); |
| + RARCloseArchive(h); |
| + free(arch_); |
| + return -dll_result; |
| } |
| |
| /* Pointless to test for encrypted files if header is already encrypted |
| * and could be read. */ |
| if (d.Flags & ROADF_ENCHEADERS) |
| goto skip_file_check; |
| - |
| if (arc->hdr.Flags & RHDF_ENCRYPTED) { |
| dll_result = extract_rar(arch_, arc->hdr.FileName, NULL); |
| - if (dll_result && dll_result != ERAR_UNKNOWN) { |
| + if (dll_result != ERAR_SUCCESS && dll_result != ERAR_UNKNOWN) { |
| RARFreeArchiveDataEx(&arc); |
| RARCloseArchive(h); |
| free(arch_); |
| @@ -1829,37 +1835,47 @@ again: |
| |
| skip_file_check: |
| RARFreeArchiveDataEx(&arc); |
| + RARCloseArchive(h); |
| + |
| list = arch_list; |
| dir_list_open(list); |
| |
| - files = 0; |
| + /* Let libunrar deal with the collection of volume parts */ |
| if (d.Flags & ROADF_VOLUME) { |
| - off_t prev_size = 0; |
| + h = RAROpenArchiveEx(&d); |
| + |
| + /* Check for fault */ |
| + if (d.OpenResult != ERAR_SUCCESS) { |
| + if (h) |
| + RARCloseArchive(h); |
| + free(arch_); |
| + return -d.OpenResult; |
| + } |
| while (1) { |
| - struct stat st; |
| - if (stat(arch_, &st)) |
| + dll_result = RARReadHeaderEx(h, &header); |
| + if (dll_result != ERAR_SUCCESS) { |
| + if (dll_result == ERAR_END_ARCHIVE) |
| + dll_result = ERAR_SUCCESS; |
| + else |
| + dll_result = ERAR_EOPEN; |
| break; |
| - if (files && st.st_size != prev_size) |
| - if (is_first_volume_by_name(arch_)) |
| - break; |
| - prev_size = st.st_size; |
| - list = dir_entry_add(list, arch_, NULL, |
| - DIR_E_NRM); |
| - ++files; |
| - RARNextVolumeName(arch_, !format); |
| + } |
| + (void)RARProcessFile(h, RAR_SKIP, NULL, NULL); |
| + list = dir_entry_add(list, header.ArcName, NULL, |
| + DIR_E_NRM); |
| } |
| + RARCloseArchive(h); |
| } else { |
| (void)dir_entry_add(list, arch_, NULL, DIR_E_NRM); |
| - files = 1; |
| + dll_result = ERAR_SUCCESS; |
| } |
| |
| - RARCloseArchive(h); |
| - free(arch_); |
| - if (!files) |
| + if (dll_result != ERAR_SUCCESS) |
| dir_list_free(arch_list); |
| + free(arch_); |
| |
| /* Do not close the list since it could re-order the entries! */ |
| - return files; |
| + return -dll_result; |
| } |
| |
| /*! |
| @@ -2638,7 +2654,6 @@ static int listrar(const char *path, struct dir_entry_list **buffer, |
| const char *arch, char **first_arch, int *final) |
| { |
| ENTER_("%s arch=%s", path, arch); |
| - |
| RAROpenArchiveDataEx d; |
| memset(&d, 0, sizeof(RAROpenArchiveDataEx)); |
| d.ArcName = (char *)arch; /* Horrible cast! But hey... it is the API! */ |
| @@ -5733,14 +5748,8 @@ int main(int argc, char *argv[]) |
| src_path_full, error_to_string(err)); |
| return err; |
| } |
| - if (ret == 0) { |
| - printf("%s: cannot find primary file for multipart archive '%s'\n", |
| - argv[0], src_path_full); |
| - return 1; |
| - } |
| } |
| |
| - |
| /* Check I/O buffer and history size */ |
| if (check_iob(argv[0], 1)) |
| return -1; |
| -- |
| 2.33.0.rc1.237.g0d66db33f3-goog |
| |
