After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771069 - batch rename: crash on Control+x
batch rename: crash on Control+x
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: File and Folder Operations
3.21.x
Other Linux
: Normal major
: 3.22
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-08 17:01 UTC by Mohammed Sadiq
Modified: 2016-09-14 08:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
batch-rename-dialog: Check if tags are used (4.33 KB, patch)
2016-09-12 09:18 UTC, Alexandru Pandelea
none Details | Review
batch-rename-dialog: Check if tags are used (2.94 KB, patch)
2016-09-12 17:52 UTC, Alexandru Pandelea
none Details | Review
batch-rename-dialog: use positions for tag handling (27.69 KB, patch)
2016-09-13 21:04 UTC, Carlos Soriano
committed Details | Review

Description Mohammed Sadiq 2016-09-08 17:01:50 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.
Comment 1 Mohammed Sadiq 2016-09-10 02:43:16 UTC
This segfault also happens on C-k (kill line) in Emacs keybindings, when the cursor precedes some tag.
Comment 2 Alexandru Pandelea 2016-09-12 09:18:58 UTC
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.
Comment 3 Carlos Soriano 2016-09-12 12:02:18 UTC
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?
Comment 4 Alexandru Pandelea 2016-09-12 17:52:04 UTC
(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.
Comment 5 Alexandru Pandelea 2016-09-12 17:52:50 UTC
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.
Comment 6 Carlos Soriano 2016-09-12 18:42:04 UTC
(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.
Comment 7 Carlos Soriano 2016-09-13 21:04:21 UTC
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.
Comment 8 Carlos Soriano 2016-09-13 21:12:33 UTC
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
Comment 9 Alexandru Pandelea 2016-09-14 08:46:18 UTC
(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 :)
Comment 10 Carlos Soriano 2016-09-14 08:57:09 UTC
Cool, glad you like it!