Subject: Re: [PATCH] Partial files are read from the beginning every time a resume is tried #435
From: Ondřej Jirman
Date: Wed, 22 May 2019 19:07:42 +0200
Hi Mert,

thanks for the patch. It looks mostly fine except for the following:

+ state_file_handle = fopen(tmp_state_path, "rb");
+ if (state_file_handle) {
+         fread(&saved_cbc_mac, sizeof(struct chunked_cbc_mac), 1, state_file_handle);
+ }
+ fclose(state_file_handle);

This will crash if open fails. fclose doesn't accept NULL argument.

Also fread may fail. I suggest you use g_file_get_contents() which handles
all the various error conditions.

The same is true for the code where you write the state file. You can
use g_file_set_contents() there. (It will also do an atomic replacement/sync
of the state file for you.)

I would also not fail the download completely if the state file can't be deleted
after everything is done.

There's also an issue of syncing. Now if you get a crash, it is possible that
data written to the file were not synced to disk, but state file was. So on
resume, you'll get a hole in the downloaded file and mega_session_download_data
will incorrectly finish the download as completed/verified.

You need to sync the data file before writing/syncing state file atomically.
For state file you can use g_file_set_contents(), and glib hopefully has
some function to sync data for the open GFile.

thank you and regards,
	Ondrej


On Wed, May 22, 2019 at 07:41:12PM +0300, Mert Dirik wrote:
> Hello,
> 
> Attached is a patch for Github issue at
> https://github.com/megous/megatools/issues/435
> 
> It is based on a my private clone (which was last merged with origin git
> revision a1c945c540dd31d61c38df87002dcbcc3af7b0dd) so I'm not sure if it
> applies cleanly but it is not an intricate patch.
> 
> I've been using it for few weeks and it seems to be working fine.
> 
> Please feel free to adapt it as you wish.
> 
> Regards,
> 
> Mert
> 
>