GNOME Bugzilla – Bug 771069
batch rename: crash on Control+x
Last modified: 2016-09-14 08:57:09 UTC
How to reproduce: 1. Select some files -> Rename 2. Select the whole text "[Original file name]" 3. Press Ctrl+X Now Nautilus crashes and core dumped error is shown. The exact message: (nautilus:31961): GLib-ERROR **: /home/sadiq/jhbuild/checkout/glib/glib/gmem.c:100: failed to allocate 18446744073709551597 bytes Trace/breakpoint trap (core dumped) I uses Emacs keybindings if that is related.
This segfault also happens on C-k (kill line) in Emacs keybindings, when the cursor precedes some tag.
Created attachment 335344 [details] [review] batch-rename-dialog: Check if tags are used There are some cases where the tag will be deleted that aren't handled in the on_key_press_event. If one of this cases occurs, then Nautilus segfaults. To avoid this issue, before getting the new names, there is made a check to be sure that the tags are actually used and if they are not there, the tags are marked as unset.
Review of attachment 335344 [details] [review]: Hey, thanks for the patch! I just pushed a refactoring of the code, so it will need adapting. About the code, we would like less places where we check tags etc, not more. So let's try to handle theses cases in a single place. I think we can do all of this still in the key-press event no?
(In reply to Carlos Soriano from comment #3) > Review of attachment 335344 [details] [review] [review]: > > Hey, thanks for the patch! > > I just pushed a refactoring of the code, so it will need adapting. > > About the code, we would like less places where we check tags etc, not more. > So let's try to handle theses cases in a single place. I think we can do all > of this still in the key-press event no? I think that we need this code because it's hard to cover all cases in which a tag can be deleted. Before, if the tag was deleted in a way that we didn't think of, then Nautilus would segfault. With this check, no matter how the tag is deleted, Nautilus won't segfault.
Created attachment 335381 [details] [review] batch-rename-dialog: Check if tags are used There are some cases where the tag will be deleted that aren't handled in the on_key_press_event. If one of this cases occurs, then Nautilus segfaults. To avoid this issue, before getting the new names, there is made a check to be sure that the tags are actually used and if they are not there, the tags are marked as unset.
(In reply to Alexandru Pandelea from comment #4) > (In reply to Carlos Soriano from comment #3) > > Review of attachment 335344 [details] [review] [review] [review]: > > > > Hey, thanks for the patch! > > > > I just pushed a refactoring of the code, so it will need adapting. > > > > About the code, we would like less places where we check tags etc, not more. > > So let's try to handle theses cases in a single place. I think we can do all > > of this still in the key-press event no? > > I think that we need this code because it's hard to cover all cases in which > a tag can be deleted. Before, if the tag was deleted in a way that we didn't > think of, then Nautilus would segfault. With this check, no matter how the > tag is deleted, Nautilus won't segfault. Well, it's hard to cover them, but we need to be explicit on what cases that can happen. I think we can come with something slightly generic. However that patch has two issues: - First it uses strstr, we want to handle tags not because it has the same text as expected, but because of the positions etc. Basically I don't think it's a good idea to play with the strings, rather use a more reliable way with positions. My idea was to just use the position all across the code, and I think we are close to that. - Second it adds another check for the tags. At some point we are repeating the process of checking if a tag is deleted all around in different ways. This needs to be in one way, in one place. So the code clearly just go through a code path for that (it's better code design and concept. In this case it's very important for being clearer and have a good maintainability). I think I can come up with something.
Created attachment 335472 [details] [review] batch-rename-dialog: use positions for tag handling When a tag is added, deleted or moved or a text is changed, inserted or deleted, we need to do update the text and tags already present based on what changed. We were checking whether a tag was present or not matching with the text rather than deterministically adding, removing or updating the positions based on the direct user interaction. Doing it in this way is a potential problem since two tags added writing manually would confuse the underlying handling, apart of being harder to have a consistent state of the tags like deleting multiple tags at once, deleting tags with text on the sides when selected, replacing tags with text, crashes when using overwrite mode instead of insertion mode, or crashes when using modifiers. Apart of making the handling more complex. This patch refactors the tag handling for using positions and direct manipulation of them instead of text.
Hey Alex, thanks for your patch! I needed to really look into the code to figure out a solution, so I end up implementing it, and as said I came up with what we discussed in the general review, handling the tags with positions and maths instead of text. This fixes this bug and actually any crasher regarding the entry and tag handling. Check it out, it looks nice with this handling! Attachment 335472 [details] pushed as 7fb5aa7 - batch-rename-dialog: use positions for tag handling
(In reply to Carlos Soriano from comment #8) > Hey Alex, thanks for your patch! > I needed to really look into the code to figure out a solution, so I end up > implementing it, and as said I came up with what we discussed in the general > review, handling the tags with positions and maths instead of text. This > fixes this bug and actually any crasher regarding the entry and tag > handling. Check it out, it looks nice with this handling! > Attachment 335472 [details] pushed as 7fb5aa7 - batch-rename-dialog: use > positions for tag handling Hey, I checked this out and now tags are handled really nicely. Thanks for fixing this :)
Cool, glad you like it!