| Summary: | ark: segfault and some more issues in recent rar engine | ||
|---|---|---|---|
| Product: | TDE | Reporter: | Alexander Golubev (Fat-Zer) <fatzer2> |
| Component: | tdeutils | Assignee: | Michele Calgaro <michele.calgaro> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | bugwatch, deloptes, fatzer2, michele.calgaro, slavek.banko |
| Priority: | P5 | ||
| Version: | R14.1.x [Trinity] | ||
| Hardware: | Other | ||
| OS: | Linux | ||
| Compiler Version: | TDE Version String: | ||
| Application Version: | Application Name: | ||
| Bug Depends on: | |||
| Bug Blocks: | 2575 | ||
| Attachments: |
backtrace
0001-ark-fix-rar-archive-support-see-Bug-2644.patch ark patch ark-fix-deaper-than-first-level-item-handle-with-Fil.patch ark-fix-deaper-than-first-level-item-handle-with-Fil.patch new patch screenshot old patch screenshot new patch new version patch test archive 0001-Ark-rar-module-fix-suppport-for-buggy-rar-versions.patch |
||
Created attachment 2653 [details]
0001-ark-fix-rar-archive-support-see-Bug-2644.patch
Pushed in commit 8cf274c (R14.1.x) and 711e6bf (R14.0.x). Minor modification on assert code (use if() instead of assert) Created attachment 2656 [details]
ark patch
Alex,
can you test the attached patch (with different achive formats) before I pushed it to git?
Your previous patch removed some code for populating/updating folder info from rar archives. While investigating that, I found the issue was in the "item()" function, which was only handling first level subfolders.
The patch seems to work fine here.
Thanks
> Your previous patch removed some code for populating/updating folder info from rar archives. While investigating that, I found the issue was in the "item()" function, which was only handling first level subfolders.
Oh, so that was a tree processing in updateItem ()...
I'm not happy with this code for several reasons:
1. It will segfault if an empty string is passed.
2. IMHO, the kdDebug () is excessive here and will produce nothing but noise due to querying nonexisting values with item() is a normal behaviour.
3. The code duplication makes it heavy, quite obscure, and /sorry for a hard word/ just crappy.
If you don't mind, I'd like to rewright this patch myself...
> Oh, so that was a tree processing in updateItem ()...
> 1. It will segfault if an empty string is passed.
> 2. IMHO, the kdDebug () is excessive here and will produce nothing but noise
> due to querying nonexisting values with item() is a normal behaviour.
> 3. The code duplication makes it heavy, quite obscure, and /sorry for a hard
> word/ just crappy.
No problem, feel free to rewrite the patch if you have a better one.
The logic to find the right item is now moved into the "item()" function, since the original version (as per current git) only looks at the first level items and do not search for further sublevels.
kdDebug() can be removed eventually.
Created attachment 2670 [details]
ark-fix-deaper-than-first-level-item-handle-with-Fil.patch
Created attachment 2671 [details]
ark-fix-deaper-than-first-level-item-handle-with-Fil.patch
Sorry for the delay... I wasn't online as such for about a month. Here is the promised patch. Thanks Alex, I have also been quite busy (and still am :-( ) and had no time for TDE at all in the past few weeks. I will test the patch when I have some time and if it works fine I will commit with your name. Hi Alex, I tested your latest patch, it is much cleaner than mine but it does not work correctly. With reference to the two attachment I will post right after, you can see that using your patch insert double elements for the top folders (for example folder "01") when reporting back the folder info that are stored at the end of the rar archive. My patch didn't insert a duplicate and instead populated the original entry. Since you were not happy with the code of my patch, I proposed I (or you if you prefer) rework either ypur patch or my patch to achieve the correct functionality and still keep the code clean enough. Please let me know (sorry for the delay of the reply, I was very busy and didn't do much work on TDE in the last couple of months). Created attachment 2678 [details]
new patch screenshot
Entries for top folders are duplicated
Created attachment 2679 [details]
old patch screenshot
Entry for top folders are not duplicated and correctly filled
Created attachment 2681 [details]
new patch
Hi Alex,
please take a look at the attached patch. It is a modifed version of your patch that now seems to work correctly.
If it is ok for you, I will push the commit to the main repo.
Comment on attachment 2681 [details]
new patch
Sorry, forget my previos patch and comment. It needs more work since it is not working for subfolders.
Will come back with a new patch version another day.
Created attachment 2682 [details]
new version patch
Ok, decided to sleep a little later and here is the new patch (without that double-nested while loop that I really didn't like).
This works on subfolders as well.
Alex, let me know if it is ok for you and I will push with your name as author.
Fixed comment and push in commit d8d57cf (r14.1.x) and f38d0cf (r14.0.x). Alex, thanks for your contribution. (In reply to Michele Calgaro from comment #16) > Fixed comment and push in commit d8d57cf (r14.1.x) and f38d0cf (r14.0.x). > Alex, thanks for your contribution. Hi, sorry for not answering for a while. Good catch that the nested loop is excess... About the dir issue, I can't reproduce it (using rar 5.3)... Could you please upload the rar file that triggers the error and a rar version you are using. Also, have you _installed_ the ark to the _system_ before running it (or done any other workarounds to properly run a kpart app from costume location)? Note that IMHO striping spaces here is unacceptable... It may be done at most on the level of rar engine, but generally shouldn't... At least the current implementation break display of files with trailing spaces... Created attachment 2684 [details] test archive Hi Alex, no problem for the delay response ;-) > About the dir issue, I can't reproduce it (using rar 5.3)... Could you please > upload the rar file that triggers the error and a rar version you are using. I have attached a test file. I had deleted the original file, but I just created another on the spot. This one should give you the problem, I can double check if you need. I am using rar 5.3 beta 2 in Debian/Stretch. NOTE: problem only shows up when you have at least 2 levels or directories. If you only have one (main) level, everything looks ok. >Also, have you _installed_ the ark to the _system_ before running it (or done >any other workarounds to properly run a kpart app from costume location)? Tested as follow: 1) compile ark code with your original patch. make install it. run ark, open archive. Alternatively you can open it from Konqueror, just make sure to close all ark instances before that. --> problem shows up 2) repeat 1) but with the fixed-up patch --> no problem >Note that IMHO striping spaces here is unacceptable... It may be done at most >on the level of rar engine, but generally shouldn't... At least the current >implementation break display of files with trailing spaces... Agree, it would break names with trailing space. On the other hand, rar seems to add(at random) trailing spaces after some of the names. Without stripping spaces, we fix problem 1 but not 2. With stripping spaces, we fix problem 2 but not 1. Prior to push the patch, I discussed with Slavek about this point (he actually raised the same objection). It looks like we can fix one problem and have to leave the other open, so we consider which event is more likely. We think the possibility of having names with trailing spaces is lower that the one where rar puts trailing spaces, so we chose the least-bad solution. If you think it would be better otherwise, we can of course consider removing the stripping part. Please let us know. (In reply to Michele Calgaro from comment #18) I too have had problems with ark in the past and it is interesting to follow this discussion. I just read your comments and I am sure you have considered removing spaces at the end of file name only. I think you have very frequently filenames with spaces, especially if you work with windows folks. Not sure if this is what is discussed here though. (In reply to Michele Calgaro from comment #18) > Created attachment 2684 [details] > test archive > > Hi Alex, no problem for the delay response ;-) > > > About the dir issue, I can't reproduce it (using rar 5.3)... Could you please > > upload the rar file that triggers the error and a rar version you are using. > I have attached a test file. I had deleted the original file, but I just > created another on the spot. This one should give you the problem, I can > double check if you need. > I am using rar 5.3 beta 2 in Debian/Stretch. > NOTE: problem only shows up when you have at least 2 levels or directories. > If you only have one (main) level, everything looks ok. > > >Also, have you _installed_ the ark to the _system_ before running it (or done > >any other workarounds to properly run a kpart app from costume location)? > Tested as follow: > 1) compile ark code with your original patch. make install it. run ark, open > archive. Alternatively you can open it from Konqueror, just make sure to > close all ark instances before that. > --> problem shows up > > 2) repeat 1) but with the fixed-up patch > --> no problem > I see... It seems to be a rar bug, which was fixed in rar 5.3 release, that's why I haven't seen it: compare the output of rar l <Archive> | cat -A For version 5.3: ====================================================== RAR 5.30 Copyright (c) 1993-2015 Alexander Roshal 18 Nov 2015$ Trial version Type RAR -? for help$ $ Archive: /tmp/mozilla_alexander0/Archive.rar$ Details: RAR 4$ $ Attributes Size Date Time Name$ ----------- --------- ---------- ----- ----$ -rw-r--r-- 3 2013-12-05 21:22 08/minni.txt$ -rw-r--r-- 3 2013-12-05 21:22 09/minni.txt$ -rw-r--r-- 43 2016-04-16 12:20 10/ciro.txt$ -rw-r--r-- 2 2013-12-05 11:44 10/pippo.txt$ -rw-r--r-- 2 2013-10-06 00:29 10/maradona.txt$ -rw-r--r-- 2 2013-10-06 00:29 10/orbo.txt$ -rw-r--r-- 3 2013-12-05 21:22 10/minni.txt$ -rw-r--r-- 2 2013-10-06 00:29 10/testqt/maradona.txt$ -rw-r--r-- 2 2013-10-06 00:29 10/testqt/maradona2.txt$ -rw-r--r-- 2 2013-10-06 00:29 10/testqt/111/ciro.txt$ -rw-r--r-- 2 2013-10-06 00:29 10/testqt/111/ciro2.txt$ -rw-r--r-- 2 2013-12-05 11:44 pippo1.txt$ -rw-r--r-- 2 2013-12-05 11:44 pippo2.txt$ -rw-r--r-- 2 2013-12-05 11:44 pippo.txt$ drwxr-xr-x 0 2016-08-25 10:37 10/testqt/111$ drwxr-xr-x 0 2016-08-25 10:37 10/testqt$ drwxr-xr-x 0 2014-01-06 00:10 08$ drwxr-xr-x 0 2014-01-06 00:10 09$ drwxr-xr-x 0 2016-08-16 23:08 10$ ----------- --------- ---------- ----- ----$ 72 19$ $ ====================================================== And 5.3.b2: ====================================================== $ RAR 5.20 Copyright (c) 1993-2014 Alexander Roshal 2 Dec 2014$ Trial version Type RAR -? for help$ $ Archive: /tmp/mozilla_alexander0/Archive.rar$ Details: RAR 4$ $ Attributes Size Date Time Name$ ----------- --------- -------- ----- ----$ -rw-r--r-- 3 05-12-13 21:22 08/minni.txt$ -rw-r--r-- 3 05-12-13 21:22 09/minni.txt$ -rw-r--r-- 43 16-04-16 12:20 10/ciro.txt $ -rw-r--r-- 2 05-12-13 11:44 10/pippo.txt$ -rw-r--r-- 2 06-10-13 00:29 10/maradona.txt$ -rw-r--r-- 2 06-10-13 00:29 10/orbo.txt $ -rw-r--r-- 3 05-12-13 21:22 10/minni.txt$ -rw-r--r-- 2 06-10-13 00:29 10/testqt/maradona.txt$ -rw-r--r-- 2 06-10-13 00:29 10/testqt/maradona2.txt$ -rw-r--r-- 2 06-10-13 00:29 10/testqt/111/ciro.txt$ -rw-r--r-- 2 06-10-13 00:29 10/testqt/111/ciro2.txt$ -rw-r--r-- 2 05-12-13 11:44 pippo1.txt $ -rw-r--r-- 2 05-12-13 11:44 pippo2.txt $ -rw-r--r-- 2 05-12-13 11:44 pippo.txt $ drwxr-xr-x 0 25-08-16 10:37 10/testqt/111$ drwxr-xr-x 0 25-08-16 10:37 10/testqt $ drwxr-xr-x 0 06-01-14 00:10 08 $ drwxr-xr-x 0 06-01-14 00:10 09 $ drwxr-xr-x 0 16-08-16 23:08 10 $ ----------- --------- -------- ----- ----$ 72 19$ $ ====================================================== just > >Note that IMHO striping spaces here is unacceptable... It may be done at most > >on the level of rar engine, but generally shouldn't... At least the current > >implementation break display of files with trailing spaces... > Agree, it would break names with trailing space. On the other hand, rar > seems to add(at random) trailing spaces after some of the names. > Without stripping spaces, we fix problem 1 but not 2. > With stripping spaces, we fix problem 2 but not 1. > Prior to push the patch, I discussed with Slavek about this point (he > actually raised the same objection). It looks like we can fix one problem > and have to leave the other open, so we consider which event is more likely. > We think the possibility of having names with trailing spaces is lower that > the one where rar puts trailing spaces, so we chose the least-bad solution. > If you think it would be better otherwise, we can of course consider > removing the stripping part. > Please let us know. I worked around the beaviour above for rar<5.3 in rar module.. see the upcoming patch. Created attachment 2685 [details]
0001-Ark-rar-module-fix-suppport-for-buggy-rar-versions.patch
Patch looks good here: rar 5.3 beta 2 still displays the extra spaces at the end and Ark behaves properly. Pushed in commit e5a3116 (r14.1.x - trunk) and 03af0c4 (r14.0.x). Tnanks for the good work Alex. Is it ok to close the bug now? :-) > Is it ok to close the bug now? :-)
Just double checking. Is it ok?
> > Is it ok to close the bug now? :-)
> Just double checking. Is it ok?
yes; just forgot to click «send» last time =)
ok thanks ;-) |
Created attachment 2652 [details] backtrace The main issue is segmentation failure (see attached backtrace). But there are other relatively smaller issues about recent implementation of rar-v5 support, see #2541. A patch is upcoming in a couple of days.