Hi Ruben, could you attach here the cbr file causing this crash please?

Hi Carlos,

I'm not attaching files because they're copyrighted material... But I think we don't need it anymore... I have more info now, and I think I found the crash cause...

The crash is related with the unrar package I'm using:
* If I have installed just the unrar-free package (upstream: https://gna.org/projects/unrar/), evince crashes trying to open the cbr.
* But if I install the non-free unrar package (upstream: http://www.rarlabs.com/) evince opens the file without problems.
* If I remove the non-free package, and then I rename the file to rar, fileroller can open the file and it's able to list the contents... it can't extract the files, but it doesn't crashes (in fact, it seems to silently ignore this)... it I try to unpack from command line I got a 'failed' message...

At this point, I was thinking this bug was related with a version of rar-format unsupported by the free unrar...

But, after some testing I found another cbr file: I can unpack it from command line using the free unrar, and if I rename it to rar I can unpack it from fileroller too... I can view it from Comix 3.6.4... but if I try to view it from evince it crashes... 

So, it was related with the application used, not with the format...

Finally, I checked the code :) and in backend/comics/comics-document.c I found:

    g_strdup ("unrar p -c- -ierr");

Here, you are assuming I have installed the non-free unrar :( and using it's syntax you are reading the stream from the stdout (using the 'p' option)... 

As the free unrar ignores this options, you got some unexpected data (actually, just plain text listing the unpacked files)... I stopped here, but I assume you try to process/display this data later and this causes the crash...

I think you can avoid this checking if file was really unpacked before trying to displaying it... or better, you should check if the installed unrar is the non-free as you are expecting, and give the user some error message if not...

Off course, it will be even better to add support for the free unrar :) but as I don't see an equivalent for the 'p' option on the free unrar, this can require a major change...

Regards,
  Ruben

PS: and thanks for your work on evince :)

We should definitely support the free unrar. Thanks Ruben. 

Created attachment 123845 [details] [review]
first proposed patch

Comment on attachment 123845 [details] [review]
first proposed patch

Hi, this is my first attempt of patch for this bug (added patch) as I told to Carlos. 

I think it needs a better detection for the avalaible program for the decompression of .rar files. Right know, if the system has the unrar command, then I assume it has the non-free version, if not, I try the free version. 

I think this works almost all the time because Debian and its derivatives renamed the free version with "unrar-free", although the original name is unrar too. So I think I have to add another conditional expression to be sure if the unrar command is the non-free version. I think I can do that checking for the string "freeware".

Hi Juanjo! and thanks for working on this report!

> Right know, if the system has the unrar command, then I assume
> it has the non-free version, if not, I try the free version

I haven't tried your patch, but I think it still fails if 'unrar-free' is installed but 'unrar-nonfree' isn't: As '/usr/bin/unrar' is created using the Debian's alternatives system, if you install just the 'unrar-free' package a '/usr/bin/unrar' symlink is created too.

Maybe we can test first for the unrar-free, and if we can't found it, test for the unrar binary... In your patch it will be something like:

====
!strcmp (mime_type, "application/x-rar")) {	    
    gchar *file_path_str_free = NULL;
    file_path_str_free = g_find_program_in_path ("unrar-free");
    if (file_path_str_free != NULL) {
	g_spawn_command_line_sync (g_strdup_printf ("unrar-free -xf %s /tmp", quoted_file), NULL, NULL, NULL, error);
	comics_document->extract_command = g_strdup ("cat /tmp/");
	list_files_command = g_strdup_printf ("unrar-free t %s", quoted_file);
    }
    else {
	gchar *file_path_str_nonfree = NULL;
	file_path_str_nonfree = g_find_program_in_path ("unrar");
	if (file_path_str_nonfree != NULL) {
	    g_free (file_path_str_nonfree);
	    comics_document->extract_command = g_strdup ("unrar p -c- -ierr");
	    list_files_command = g_strdup_printf ("unrar vb -c- -- %s", quoted_file);
	}
    }
    comics_document->regex_arg = FALSE;
====

Anyway, it seems to be too debian-specific... Once completed, it should be forwarded to the debian package maintainers so it can be included until a more general solution is prepared, maybe parsing the 'unrar --version' output looking for the 'freeware' string, as you already suggested...

I have just another comment about your patch, but again: I haven't tested it yet, so maybe the following questions are irrelevant... sorry :)

We are using "unrar-free -xf %s /tmp" and "cat /tmp/" for the unrar-free option...  Is this safe? Isn't better to use a tmp subdir or something? I mean:
What if they are some other images in /tmp/ before the extraction? Will they be displayed too? 
What if I open many cbr files at the same time? 
What if the file is open by one user, and then by another one? We will have some permissions issues on overwritting, no?

Uhmm, are the temporal files erased on close?

Thanks again :)
Have a Happy New Year!

Hi,

I've re-written the patch. It fixes this bug, but other issues too, including bug 565174. It is a more general fix for decompressing cbr, cbz and cb7 files. I've created more functions in order to make a more comprehensive code.

Yes, you're right:

a) Now I don't use /tmp directly, I use instead g_get_tmp_dir()
b) Yes, I use a new directory for decompresing every comic file whose name is the MD5 of its filename.

Right now, I don't erase the temporal files, but I'm going to check if I can do that (I hope so)

Thank you for your comments.

   -- Juanjo
Feliz año nuevo para ti también ;-)

PS: I'm going to post the patch I'm working on if you want to take a look.

Created attachment 126310 [details] [review]
Patch for decompressing cbr, cbz and cb7 comic formats. Fixes bug 552074 and bug 565174.

It adds new functions to make a better structured code.

Thanks a lot

Coding style is not always perfect btw in part of spacing inside function calls.

Created attachment 126473 [details] [review]
More polished patch. It fixes some memory leaks.

It would be nice if distribution packagers could add decompress programs as recommended other than unrar.

by now, with this patch, there more options to recommend

1) rar from RARLabs (commercial, shareware, non-free) for cbr files. 
2) unrar from RARLabs (freeware, non-free) for cbr files. The unRAR license stablishes it cannot be used to re-create the RAR compression algorithm
2) unrar from Gna! (unrar-free) (free software, GPLv2)
3) unzip from InfoZIP for cbz files (free software)
4) 7zr (p7zip) for cb7 files (free software, LGPL)
5) 7za (p7zip-full) for cbz and cb7 files (free software, LGPL)
6) 7z (p7zip-full) + Rar29.so (p7zip-rar) for cbr, cbz and cb7 files (free software, LGPL, for 7z and the non-free unRAR license for Rar29.so)

Created attachment 128798 [details] [review]
Removes the use of cat. Shorter error messages at decompressing on the tmp.  Add check for cbr plugin presence. 7z with rar plugin takes precedence over Gna unrar for cbr files.

- Removes the use of cat to easier cross-platform development. 
- Shorter error messages at decompressing on the tmp.  
- Add check for rar plugin presence of 7z. 
- 7z with rar plugin takes precedence over Gna unrar for cbr files.

Created attachment 129594 [details] [review]
free unrar support (also reported to KaL by email for review)

- This patch _only_ adds support for unrar free for easier review (It removes other fixes that hopefully will be submitted after the acceptance of this patch). Fix bug 552074
- Adds better error reporting to the previous versions

Comment on attachment 129594 [details] [review]
free unrar support (also reported to KaL by email for review)

This patch introduces a lot of translatable strings, and we are now in string freeze, so we should try to remove them somehow. More comments blow.


>+
>+static gboolean 
>+comics_decompress_temp_dir (const gchar *command_decompress_tmp,
>+		      const gchar	*comic_decompress_command, 
>+		      GError 		**error)

it's seems there are problems with the indentation here. Do not use tabs. 

>+{
>+	
>+	gboolean success;
>+	gchar *std_out;
>+	GError *err = NULL;
>+	gint retval;
>+
>+	success = g_spawn_command_line_sync (command_decompress_tmp, 
>+		&std_out, NULL, &retval, &err);

indentation problems here again. 

>+	/* Gna! unrar displays a line for every file extracted, ending with 
>+	 * OK or Failed, and a final line with the total number of errors, 
>+	 * eg. 81 Failed   
>+	 */
>+	
>+	if (success && WIFEXITED (retval) && 
>+		!g_str_has_suffix (std_out,"Failed\n")) {		
>+		g_free (std_out);	
>+		return TRUE;
>+	}
>+	else {

use } else { instead

>+	/* FIXME I don't know if we have to put code on this place in order 
>+	 * to delete the directory and files created so far. The fact is the 
>+	 * directory remains on the temporary dir, even when there is code
>+	 * for this task on comics_document_finalize() */
>+		if (!success ) {
>+			g_set_error (error,
>+				EV_DOCUMENT_ERROR,
>+				EV_DOCUMENT_ERROR_INVALID,
>+				_("Error launching the %s command for decompressing the \
>+				comic book in the temporary directory -- %s."),	
>+				comic_decompress_command,		
>+				err->message);
>+				g_error_free (err);
>+		} else {
>+			g_set_error (error,
>+				EV_DOCUMENT_ERROR,
>+				EV_DOCUMENT_ERROR_INVALID,
>+				_("The command %s failed at decompressing the comic book \
>+				in the temporary directory."), 
>+				comic_decompress_command);
>+		}		

here we can probably do something like 

if () {
} else if () {
} else {
}

I think it's more readable and we save an indentation level.

>+		g_free (std_out);
>+		return FALSE;
>+	}
>+}

>+	/* Gna! unrar */
>+	if (g_str_has_suffix (comics_document->selected_command, "unrar-free") ||
>+		(g_str_has_suffix (comics_document->selected_command, "unrar") &&
>+		 comics_document->flag_use)) {
>+		
>+		comics_document->flag_temp = TRUE;
>+		
>+		comics_document->dir = g_strdup_printf ("%s/%s", g_get_tmp_dir(),
>+					g_compute_checksum_for_string (G_CHECKSUM_MD5, 
>+					comics_document->archive, -1));		

in libdocument/ev-file-helpers.[ch] we have functions for handling temp stuff. Use ev_temp_dir() instead of g_get_temp_dir(), for example. 

>+static gboolean 
>+comics_check_decompress_command (gchar *mime_type, 
>+		      ComicsDocument *comics_document,
>+		      GError		**error)	
>+{
>+	gboolean success;
>+	gchar *std_out, *std_err;
>+	gint retval;
>+	GError *err;
>+

you should always initialize GError variables to NULL. 

>+	/* FIXME, use proper cbr/cbz mime types once they're
>+	 * included in shared-mime-info */
>+	
>+	if (!strcmp (mime_type, "application/x-cbr") ||
>+	    !strcmp (mime_type, "application/x-rar")) {
>+	/* The RARLAB provides a no-charge proprietary (freeware) 
>+	 * decompress-only client for Linux called unrar. Another 
>+	 * option is a GPLv2-licensed command-line tool developed by 
>+	 * the Gna! project. Confusingly enough, the open source RAR 
>+	 * decoder is also named unrar. However, some distributions, 
>+	 * like Debian, rename this free option as unrar-free.
>+	 * */    	
>+		comics_document->selected_command = g_find_program_in_path ("unrar");
>+		if (comics_document->selected_command) {
>+			/* We only use std_err to avoid printing error messages on 
>+			the terminal */
>+			success = g_spawn_command_line_sync (comics_document->selected_command, 
>+				&std_out, &std_err, &retval, &err);
>+			if (success && WIFEXITED (retval)) {
>+				if (std_out){
>+					if (g_strrstr (std_out,"freeware") != NULL) {
>+	/* The RARLAB freeware client */
>+						comics_document->flag_use = FALSE;
>+						g_free (std_out);
>+						g_free (std_err);
>+						return TRUE;
>+					} else {
>+	/* The Gna! free software client */

more indentation problems with thecomments here. 

> static gboolean
> comics_document_load (EvDocument *document,
> 		      const char *uri,
>@@ -109,7 +375,7 @@
> {
> 	ComicsDocument *comics_document = COMICS_DOCUMENT (document);
> 	GSList *supported_extensions;
>-	gchar *list_files_command = NULL, *std_out, *quoted_file;
>+	gchar *std_out;
> 	gchar *mime_type;
> 	gchar **cbr_files;
> 	gboolean success;
>@@ -134,48 +400,20 @@
> 		return FALSE;
> 	}
> 
>-	quoted_file = g_shell_quote (comics_document->archive);
>+	if (!comics_check_decompress_command (mime_type, comics_document, error)) {	
>+			g_free (mime_type);
>+			return FALSE;
> 
>-	/* FIXME, use proper cbr/cbz mime types once they're
>-	 * included in shared-mime-info */
>-	if (!strcmp (mime_type, "application/x-cbr") ||
>-	    !strcmp (mime_type, "application/x-rar")) {
>-		comics_document->extract_command =
>-			g_strdup ("unrar p -c- -ierr");
>-		list_files_command =
>-			g_strdup_printf ("unrar vb -c- -- %s", quoted_file);
>-		comics_document->regex_arg = FALSE;
>-	} else if (!strcmp (mime_type, "application/x-cbz") ||
>-		   !strcmp (mime_type, "application/zip")) {
>-		comics_document->extract_command =
>-			g_strdup ("unzip -p -C");
>-		list_files_command = 
>-			g_strdup_printf ("zipinfo -1 -- %s", quoted_file);
>-		comics_document->regex_arg = TRUE;
>-	} else if (!strcmp (mime_type, "application/x-cb7")) {
>-		comics_document->extract_command =
>-			g_strdup ("7zr x -so");
>-		list_files_command = 
>-			g_strdup_printf ("7zr l -- %s", quoted_file);
>-		comics_document->regex_arg = TRUE;
>-	} else {
>-		g_set_error (error,
>-			     EV_DOCUMENT_ERROR,
>-			     EV_DOCUMENT_ERROR_INVALID,
>-			     _("Not a comic book MIME type: %s"),
>-			     mime_type);
>-		g_free (mime_type);
>-		g_free (quoted_file);
>-		return FALSE;
>+	} else if (!comics_generate_command_lines (comics_document, error)) {
>+			g_free (mime_type);
>+			return FALSE;
> 	}
> 
> 	g_free (mime_type);
>-	g_free (quoted_file);
> 
> 	/* Get list of files in archive */
>-	success = g_spawn_command_line_sync (list_files_command,
>+	success = g_spawn_command_line_sync (comics_document->list_command,
> 					     &std_out, NULL, &retval, error);
>-	g_free (list_files_command);
> 
> 	if (!success) {
> 		return FALSE;
>@@ -264,43 +502,62 @@
> 	gboolean success, got_size = FALSE;
> 	gint outpipe = -1;
> 	GPid child_pid = -1;
>+	gssize bytes;
>+	GdkPixbuf *pixbuf;
>+	gchar *filename;
>+	ComicsDocument *comics_document = COMICS_DOCUMENT (document);
>+	
>+	/* FIXME Add Error Reporting */
>+	if (!comics_document->flag_temp) {
>+		
>+		argv = extract_argv (document, page->index);
>+		success = g_spawn_async_with_pipes (NULL, argv, NULL,
>+						    G_SPAWN_SEARCH_PATH | G_SPAWN_STDERR_TO_DEV_NULL,
>+						    NULL, NULL,
>+						    &child_pid,
>+						    NULL, &outpipe, NULL, NULL);
>+		g_strfreev (argv);
>+		g_return_if_fail (success == TRUE);
> 
>-	argv = extract_argv (document, page->index);
>-	success = g_spawn_async_with_pipes (NULL, argv, NULL,
>-					    G_SPAWN_SEARCH_PATH | G_SPAWN_STDERR_TO_DEV_NULL,
>-					    NULL, NULL,
>-					    &child_pid,
>-					    NULL, &outpipe, NULL, NULL);
>-	g_strfreev (argv);
>-	g_return_if_fail (success == TRUE);
>+		loader = gdk_pixbuf_loader_new ();
>+		g_signal_connect (loader, "area-prepared",
>+				  G_CALLBACK (get_page_size_area_prepared_cb),
>+				  &got_size);
> 
>-	loader = gdk_pixbuf_loader_new ();
>-	g_signal_connect (loader, "area-prepared",
>-			  G_CALLBACK (get_page_size_area_prepared_cb),
>-			  &got_size);
>-
>-	while (outpipe >= 0) {
>-		gssize bytes = read (outpipe, buf, 1024);
>+		while (outpipe >= 0) {
>+			bytes = read (outpipe, buf, 1024);
> 		
>-		if (bytes > 0)
>+			if (bytes > 0)
> 			gdk_pixbuf_loader_write (loader, buf, bytes, NULL);
>-		if (bytes <= 0 || got_size) {
>-			close (outpipe);
>-			outpipe = -1;
>-			gdk_pixbuf_loader_close (loader, NULL);
>+			if (bytes <= 0 || got_size) {
>+				close (outpipe);
>+				outpipe = -1;
>+				gdk_pixbuf_loader_close (loader, NULL);
>+			}
> 		}
>-	}
> 
>-	if (gdk_pixbuf_loader_get_pixbuf (loader)) {
>-		GdkPixbuf *pixbuf = gdk_pixbuf_loader_get_pixbuf (loader);
>+		if (gdk_pixbuf_loader_get_pixbuf (loader)) {
>+			pixbuf = gdk_pixbuf_loader_get_pixbuf (loader);
>+			if (width)
>+				*width = gdk_pixbuf_get_width (pixbuf);
>+			if (height)
>+				*height = gdk_pixbuf_get_height (pixbuf);
>+		}
>+
>+		g_spawn_close_pid (child_pid);
>+		g_object_unref (loader);
>+	} else {
>+		filename = g_strdup_printf ("%s/%s", comics_document->dir, 	
>+										(char*) g_slist_nth_data (
>+											comics_document->page_names, 
>+											page->index));

use g_build_filename() to build paths.

>+		pixbuf = gdk_pixbuf_new_from_file (filename, NULL);
>+		g_free (filename);
> 		if (width)
> 			*width = gdk_pixbuf_get_width (pixbuf);
> 		if (height)
>-			*height = gdk_pixbuf_get_height (pixbuf);
>+			*height = gdk_pixbuf_get_height (pixbuf);		

what's the change here? 

> 	}
>-
>-	g_spawn_close_pid (child_pid);
>-	g_object_unref (loader);
> }


> 
>@@ -384,11 +663,49 @@
> 	gdk_pixbuf_loader_set_size (loader, w, h);
> }
> 
>+/* Removes a directory recursively 
>+ * Returns  0 if it was successfully deleted,
>+ *         -1 if an error occurred 				*/
>+static int 
>+comics_remove_dir (gchar *path_name) 
>+{
>+	GDir  *content_dir;
>+	const gchar *filename;
>+	int   out;
>+	
>+	if (g_file_test (path_name, G_FILE_TEST_IS_DIR)) {		
>+		content_dir = g_dir_open  (path_name, 0, NULL);
>+		filename  = g_dir_read_name (content_dir);
>+		out = 0;
>+		while (filename) {			
>+			if (comics_remove_dir (g_strdup_printf("%s/%s", path_name, 
>+				filename)) < out)
>+					out = -1;
>+			filename = g_dir_read_name (content_dir);
>+		}
>+		g_dir_close (content_dir);
>+		
>+		if (out == 0)
>+			return (g_remove (path_name));
>+		else
>+			return -1;
>+	}	
>+	else 							/* G_FILE_TEST_IS_REGULAR */
>+		return (g_remove (path_name));
>+}

you don't need to do that. You already knowns all the paths, so just remove them, and then remove the directory that should be empty. 

> static void
> comics_document_finalize (GObject *object)
> {
> 	ComicsDocument *comics_document = COMICS_DOCUMENT (object);
>-
>+	
>+	if (comics_document->flag_temp) {
>+		if (g_file_test (comics_document->dir, G_FILE_TEST_EXISTS))

you are checking this twice. You don't need to check this indeed, just try to remove the directory and handle the returned error. This way you avoid a possible race condition too. 

I still see this patch too complicated just to support the free unrar.

Created attachment 130221 [details] [review]
Fixed patch

KaL, thank you for your tips, there are quite certain. AFAIK, everything you commented is fixed. 

> 			*width = gdk_pixbuf_get_width (pixbuf);
> 		if (height)
>-			*height = gdk_pixbuf_get_height (pixbuf);
>+			*height = gdk_pixbuf_get_height (pixbuf);		

what's the change here? 

This is a diff issue. I just added a whole new alternative "else" branch for gdk_pixbuf_new_from_file, that happens to be very similar to the "if" branch.

About the complexity of this patch, I only have to say it is more complicated that it seems. Firstly, I must perform a disambiguation between unrar "free" and unrar "freeware". Secondly, the use of unrar-free implies the creation and deletion of a directory on the temporal directory, reading the images from files instead of reading from a pipe. On the other side, I try to add a better error reporting and this means more lines of code. And finally, some of the new lines are just old lines with new indexation (390-450, 482-535)

About the strings, yes, there are new string and we're on string freeze. I'm sure you have the best answer to this :-)

*** Bug 575106 has been marked as a duplicate of this bug. ***

*** Bug 578141 has been marked as a duplicate of this bug. ***

I'm cleaning the code. I hope to finish soon :)

Created attachment 133375 [details] [review]
Shorter and better patch. I did some cleaning and small improvements.

Ready for review guys !!!. I look forward to apply it but any suggested change will be welcome :)

I applied this one with minor modifications, mostly ok I think. I think Juango will fix other issue but they should be separate bugs then.

There are lots of identation issues in that patch. 

I think translatable error messages should be reviewed too.

And there are several memory leaks:

 * g_path_get_basename () returns a new allocated string. 
 * g_compute_checksum_for_string () also returns a new allocated string
 * in comics_remove_dir() g_build_filename() is leaking for every iteration of then loop

Thank you KaL for your comments. Very helpful, I hope learned from this. I attach a patch that fixes the memory leaks and simplifies an error string.

About the indentation issues I have to learn more about it. I ask on the IRC if someone can format the patch for learning from the diff. I guess the worst part are the headers functions.... If nodody replies me I'll ask (or ask you) again :) 

These are the added translatable strings. I'll ask a native speaker on the IRC to correct them. I write here to easy the correction process:

_("Error launching the %s command for decompressing the comic book into the temporary directory.")
_("The command %s failed at decompressing the comic book in to temporary directory.")
_("The command %s does not end normally.")
_("Failed to create a directory on the temporary directory.")
_("Internal error configuring the command for decompressing the comic book file")

Created attachment 133616 [details] [review]
Fixes the memory leaks and simplies an error string

These may be better:

_("Error launching %s in order to decompress the comic book.")
_("%s failed at decompressing the comic book.")
_("The command %s did not end normally.")
_("Failed to create a temporary directory.")
_("Internal error configuring the decompression of the comic book
file")

However, these messages seem too verbose.

This patch committed with small modifications, I made error messages translated and removed duplicated warnings, the real message will be displayed in URI anyhow. Feel free to submit any more corrections please.

hmm, I don't think we should mark g_warning messages as translatable. Those are not messages for users, but to help developers while debugging, they are not useful at all if they are not in english. 

Created attachment 133865 [details] [review]
Fixes style issues. 

Lines starting after ( when calling functions, if possible. Added '' for the error messages that include a command name. Added '' for directories. Added FIXME Comment for adding Error reporting. Created errsv for using errno value.

Created attachment 133866 [details] [review]
utf-8 quotes and no comments

Lines starting after ( when calling functions, if possible. Added utf-8 quotes “” for the error messages that include a command name. Added “” for directories too. Created errsv for using errno v

In, please create new bugs, this one is closed