Edrusb / DAR

DAR - Disk ARchive
http://dar.linux.free.fr/
GNU General Public License v2.0
138 stars 19 forks source link

Dar possibly ignores files with long names #50

Closed jgoerzen closed 1 year ago

jgoerzen commented 1 year ago

Hi Denis,

I've taken over maintaining Dar in Debian, and am going over old Dar bugs in the Debian bug-tracking system.

In there, we have https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=912387, which says "dar doesn't backup maximum length files" and was reported against Dar 2.5.8. It inclues this proposed patch:

--- a/src/libdar/etage.cpp
+++ b/src/libdar/etage.cpp
@@ -156,8 +156,11 @@ namespace libdar
            ret->d_name[max_alloc_filename] = '\0'; // yes, one byte is allocated for the terminal zero
            if(strlen(ret->d_name) >= max_alloc_filename)
            {
-           ui.warning(tools_printf(gettext("Filename provided by the operating system seems truncated in directory %s ignoring it: %s"), dirname, ret->d_name));
-           continue;
+           bool ignore = strlen(ret->d_name) > max_alloc_filename;
+           ui.warning(tools_printf(gettext("Filename provided by the operating system seems truncated in directory %s%s: %s"),
+               dirname, ignore ? " ignoring it" : "", ret->d_name));
+           if(ignore)
+               continue;
            }
 #else
        while(!is_cache_dir && (ret = readdir(tmp)) != nullptr)

I see there is similar code present in 2.7.9:

        while(!is_cache_dir && (readdir_r(tmp, ret, &dbldrt) == 0) && dbldrt != nullptr)
        {
            ret->d_name[max_alloc_filename] = '\0'; // yes, one byte is allocated for the terminal zero
            if(strlen(ret->d_name) >= max_alloc_filename)
            {
            ui.message(tools_printf(gettext("Filename provided by the operating system seems truncated in directory %s, storing filename as is: %s"), dirname, ret->d_name));
            continue;
            }

I must admit to being a bit unclear about the logic here (especially where max_alloc_filename comes from). However, it looks like the difference between the code in 2.7.9 and the proposed patch hinges on the difference between strlen(ret->d_name) => max_alloc_filename and strlen(ret->dname) > max_alloc_filename.

I also wonder if the message in 2.7.9 is accurate; it says "storing filename as is" but the subsequent continue implies that maybe it's not actually storing the filename at all, I think.

Thanks!

Edrusb commented 1 year ago

Hi John

after code review I come to the following conclusion:

Thus, the bug report you mention is also obsolete and does not more reflect any problem with libdar, if ever this was not a OS inconsistency (I could not find any error in the code related to readdir_r, but would need test further to be sure, which I will not do because readdir_r is deprecated).

I will rather cleanup the code from these old stuff which make it complex and difficult to read, but this should not change anything in libdar's behavior

Best Regards, Denis

Edrusb commented 1 year ago

code cleaned up with commit 5b58f375e8585f35258128cf9da334f8b33c64ee

Edrusb commented 1 year ago

closing issue with release 2.7.10