On Thu, 04 Aug 2016 17:08:20 +0200 Tim Ruehsen <[email protected]> wrote:
> On Thursday, August 4, 2016 12:40:49 PM CEST Matthew White wrote: > > On Thu, 04 Aug 2016 12:08:50 +0200 > > > > Tim Ruehsen <[email protected]> wrote: > > > On Wednesday, August 3, 2016 2:40:14 PM CEST Matthew White wrote: > > > > On Wed, 03 Aug 2016 14:05:06 +0200 > > > > > > > > Tim Ruehsen <[email protected]> wrote: > > > > > On Tuesday, August 2, 2016 11:27:28 AM CEST Matthew White wrote: > > > > > > On Mon, 01 Aug 2016 16:32:30 +0200 > > > > > > > > > > > > Tim Ruehsen <[email protected]> wrote: > > > > > > > On Saturday, July 30, 2016 9:41:48 PM CEST Matthew White wrote: > > > > > > > > Hello! > > > > > > > > > > > > > > > > I think that sometimes it could help to keep downloaded > > > > > > > > Metalink's > > > > > > > > files > > > > > > > > which have a bad hash. > > > > > > > > > > > > > > > > The default wget behaviour is to delete such files. > > > > > > > > > > > > > > > > This patch provides a way to keep files which have a bad hash > > > > > > > > through > > > > > > > > the > > > > > > > > option --keep-badhash. It appends the suffix .badhash to the > > > > > > > > file > > > > > > > > name, > > > > > > > > except without overwriting existing files. In the latter case, > > > > > > > > an > > > > > > > > unique > > > > > > > > suffix is appended after .badhash. > > > > > > > > > > > > > > > > I made this patch working on the following branch: > > > > > > > > master (latest 20cac2c5ab3d63aacfba35fb10878a2d490e2377) > > > > > > > > git://git.savannah.gnu.org/wget.git > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > Hi Matthew, > > > > > > > > > > > > > > good work ! > > > > > > > > > > > > > > While your FSF assignment is underway (PM), we can continue > > > > > > > polishing. > > > > > > > > > > > > > > Didn't test your code yet, but from what I see, there are just > > > > > > > these > > > > > > > peanuts: > > > > > > > > > > > > > > 1. > > > > > > > + bhash = malloc (strlen (name) + strlen (".badhash") + 1); > > > > > > > + strcat (strcpy (bhash, name), ".badhash"); > > > > > > > > > > > > > > Please consider using concat_strings() from util.h. > > > > > > > And please leave an empty line between var declaration and code - > > > > > > > just > > > > > > > for > > > > > > > readability. > > > > > > > > > > > > > > 2. > > > > > > > Please add 'link' and 'unlink' to bootstrap.conf. Gnulib will > > > > > > > emulate > > > > > > > these on platforms where they are not available (or have a > > > > > > > slightly > > > > > > > different behavior). I guess we simply forgot 'unlink' when we > > > > > > > switched > > > > > > > to gnulib. > > > > > > > > > > > > > > 3. > > > > > > > The (GNU) commit messages should ideally look like: > > > > > > > > > > > > > > one line of short description > > > > > > > <empty line> > > > > > > > file changes > > > > > > > <empty line> > > > > > > > long description > > > > > > > > > > > > > > For example: > > > > > > > Add new option --keep-badhash > > > > > > > > > > > > > > * src/init.c: Add keepbadhash > > > > > > > * src/main.c: Add keep-badhash > > > > > > > * src/options.h: Add keep_badhash > > > > > > > * doc/wget.texi: Add docs for --keep-badhash > > > > > > > * src/metalink.h: Add prototypes badhash_suffix(), > > > > > > > badhash_or_remove() > > > > > > > * src/metalink.c: New functions badhash_suffix(), > > > > > > > badhash_or_remove(). > > > > > > > > > > > > > > (retrieve_from_metalink): Call append .badhash() > > > > > > > > > > > > > > With --keep-badhash, append .badhash to Metalink's files with > > > > > > > checksum > > > > > > > mismatch, except without overwriting existing files. > > > > > > > > > > > > > > Without --keep-badhash, remove downloaded files with checksum > > > > > > > mismatch > > > > > > > (this conforms to the old behaviour). > > > > > > > > > > > > > > [This also applies to to your other patches] > > > > > > > > > > > > > > 4. > > > > > > > Please not only write 'keepbadhash', but also what you did (e.g. > > > > > > > remove, > > > > > > > rename, add, ...), see my example above. > > > > > > > > > > > > > > Those ChangeLog entries should allow finding changes / faults / > > > > > > > regressions > > > > > > > etc. even when a versioning system is not at hand, e.g. when > > > > > > > unpacking > > > > > > > the > > > > > > > sources from a tarball. (Not debatable that consulting commit > > > > > > > messages > > > > > > > directly is much more powerful.) > > > > > > > > > > > > > > 5. > > > > > > > You write "except without overwriting existing files", maybe you > > > > > > > should > > > > > > > mention appending a counter instead of overwriting existent files > > > > > > > !? > > > > > > > > > > > > > > > > > > > > > Regards, Tim > > > > > > > > > > > > Thanks Tim! > > > > > > > > > > > > I really needed your guidance. I sent the modified patches to > > > > > > [email protected]. > > > > > > > > > > > > I believe there are more things to fix. > > > > > > > > > > > > Consider the following after applying the attached patch: > > > > > > * src/metalink.c (retrieve_from_metalink): line 124: If the download > > > > > > is > > > > > > interrupted (output_stream isn't NULL), and there are more urls for > > > > > > the > > > > > > same file (we are still in the download loop), switching to the next > > > > > > url > > > > > > should resume the download (instead than start it over). > > > > > > > > > > > > In this patch I added a fix to rename/remove the fully downloaded > > > > > > file > > > > > > (output_stream is NULL), but there was an error (probably checksum) > > > > > > and > > > > > > we > > > > > > are still in the download loop (more urls for the same file). But as > > > > > > said > > > > > > before, switching to the next url should continue the download: * > > > > > > src/metalink.c (retrieve_from_metalink): line 131: Rename/remove > > > > > > fully > > > > > > downloaded file on error > > > > > > > > > > > > I still have to investigate the problem... > > > > > > > > > > So, I wait with 0001-... !? > > > > > > > > > > 0002-... has been pushed (extended with 'symlink'). > > > > > > > > > > Thanks for your contribution. > > > > > > > > > > Tim > > > > > > > > Ok for 0002. Thanks. > > > > > > > > There's no problem applying the 0001. > > > > > > Applied and pushed ! > > > > > > > The "if the stream got interrupted, then restart the download with the > > > > next > > > > url" (output_stream isn't NULL) was already there before the patch 0001. > > > > > > > > [debug is required to know when output_stream isn't NULL] > > > > > > > > commit 7fad76db4cdb7a0fe7e5aa0dd88f5faaf8f4cdc8 > > > > * src/metalink.c (retrieve_from_metalink): line 124: 'if > > > > (output_stream)' > > > > remove file and restart download with next url > > > > > > > > With the 0001, if --keep-badhash is used the file is renamed instead > > > > than > > > > removed, even when "output_stream is NULL". > > > > > > > > I have to look through the "stream interrupted" situation. > > > > > > > > I'm guessing that the download is not resumed. > > > > > > > > What do you suggest? > > > > > > We need a document where we define wanted behavior, create tests and > > > amend the code. > > > > > > Regards, Tim > > > > Ok, I just documented a test. See the attached tarball. > > > > Also there is a patch attached (there's a copy in the tarball too), > > providing the suggested bugfix. > > I made that test without your patch => crash due to badhash_or_remove(NULL). The SIGSEGV is nasty: * src/metalink.c (retrieve_from_metalink: line 166): 'output_stream = fopen (mfile->name, "ab");': Introduced with commit e0b60fd0736c7f25642b0cb7cdfc0dd74f1d6d6f: Triggers when --continue is used; SIGSEGV since commit 943a6d585ff4a0afd974ab2e4838c9d2a63cf21a: Appending to file leads to badhash_suffix(NULL) or badhash_or_remove(NULL) segmentation fault Bugfix A: I attached a patch to avoid the badhash_suffix(NULL) and badhash_or_remove(NULL) segmentation faults. But I'm still investigating the problem caused by appending to file 'output_stream = fopen (mfile->name, "ab");'. Bugfix B: '[Bug-wget] [PATCH] Support metalink:file elements with a "path/file" format' http://lists.gnu.org/archive/html/bug-wget/2016-08/msg00021.html This supersedes Bugfix A, it enables metalink:file "path/file" name format, also fixes the bug you saw in valgrind log (read below). You discussed the dangers of Bugfix B, I will answer soon: http://lists.gnu.org/archive/html/bug-wget/2016-08/msg00046.html > With your patch, valgrind says: > > ==15068== Syscall param open(filename) points to unaddressable byte(s) > ==15068== at 0x6848F40: __open_nocancel (syscall-template.S:84) > ==15068== by 0x67DFF7D: _IO_file_open (fileops.c:221) > ==15068== by 0x67E009F: _IO_file_fopen@@GLIBC_2.2.5 (fileops.c:328) > ==15068== by 0x67D4F03: __fopen_internal (iofopen.c:86) > ==15068== by 0x434FAE: retrieve_from_metalink (metalink.c:184) > ==15068== by 0x406C63: main (main.c:2025) > ==15068== Address 0x0 is not stack'd, malloc'd or (recently) free'd > ==15068== > > Can you reproduce this ? The bug you describe in the valgrind log (not the segmentation fault) it's there since commit 20cac2c5ab3d63aacfba35fb10878a2d490e2377. It's caused by leaving filename unset: * src/metalink.c (retrieve_from_metalink): src/utils.c (unique_create): Cannot handle "path/file" format, so filename is left unset * src/metalink.c (retrieve_from_metalink): Since commit e0b60fd0736c7f25642b0cb7cdfc0dd74f1d6d6f: With --continue, filename is left unset Bugfix B (discussed above): '[Bug-wget] [PATCH] Support metalink:file elements with a "path/file" format' http://lists.gnu.org/archive/html/bug-wget/2016-08/msg00021.html This describes more a user's side of view of the problem: http://lists.gnu.org/archive/html/bug-wget/2016-08/msg00052.html You discussed the dangers of enabling "path/file" (see Bugfix B above): http://lists.gnu.org/archive/html/bug-wget/2016-08/msg00046.html Vanilla commit 20cac2c5ab3d63aacfba35fb10878a2d490e2377 (before my patches) $ valgrind --leak-check=full wget [-c] --input-metalink=test-pathfile-sha256.meta4 Bug: * src/metalink.c (retrieve_from_metalink): src/utils.c (unique_create): Cannot handle "path/file" format, so filename is left unset retrieve_from_metalink (metalink.c:171): local_file = fopen (filename, "rb"); main (main.c:2022): retr_err = retrieve_from_metalink (metalink); ==15784== Syscall param open(filename) points to unaddressable byte(s) ==15784== at 0x69D4330: __open_nocancel (syscall-template.S:84) ==15784== by 0x696C3AD: _IO_file_open (fileops.c:221) ==15784== by 0x696C4F2: _IO_file_fopen@@GLIBC_2.2.5 (fileops.c:328) ==15784== by 0x6960DA3: __fopen_internal (iofopen.c:86) ==15784== by 0x44952E: retrieve_from_metalink (metalink.c:171) ==15784== by 0x430BA4: main (main.c:2022) ==15784== Address 0x0 is not stack'd, malloc'd or (recently) free'd ==15784== ... ==15784== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 0 from 0) $ valgrind --leak-check=full wget [-c] --input-metalink=test-file-sha256.meta4 ==15792== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) > > Tim I also attached the tests I have used. PS: gosh, the testing took some serious time... Soon, Matthew -- Matthew White <[email protected]>
>From 36a6809e6786797498e5693f19cafd82dd7ab034 Mon Sep 17 00:00:00 2001 From: Matthew White <[email protected]> Date: Fri, 5 Aug 2016 05:30:18 +0200 Subject: [PATCH] Bugfix: Avoid segmentation fault in Metalink module * src/metalink.c (badhash_suffix, badhash_or_remove): If name is NULL, DEBUGP the error and return the function The bug is generated calling badhash_suffix() and badhash_or_remove() with filename unset as argument. In the following conditions filename is left unset: * src/metalink.c (retrieve_from_metalink): src/utils.c (unique_create): Cannot handle "path/file" formats, so filename is left unset * src/metalink.c (retrieve_from_metalink): Since commit e0b60fd0736c7f25642b0cb7cdfc0dd74f1d6d6f: With --continue, filename is left unset Bug causing factor: * src/metalink.c (retrieve_from_metalink: line 166): 'output_stream = fopen (mfile->name, "ab");': Appending to file seems to generate the segmentation fault when filename is unset --- src/metalink.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/metalink.c b/src/metalink.c index fd6d0e2..2ca0b18 100644 --- a/src/metalink.c +++ b/src/metalink.c @@ -563,6 +563,13 @@ badhash_suffix (char *name) { char *bhash, *uname; + /* Bugfix: Avoid segmentation fault if name is NULL. */ + if (!name) + { + DEBUGP (("badhash_suffix: name is NULL\n")); + return; + } + bhash = concat_strings (name, ".badhash", (char *)0); uname = unique_name (bhash, false); @@ -586,6 +593,13 @@ badhash_suffix (char *name) void badhash_or_remove (char *name) { + /* Bugfix: Avoid segmentation fault if name is NULL. */ + if (!name) + { + DEBUGP (("badhash_suffix: name is NULL\n")); + return; + } + if (opt.delete_after || !opt.keep_badhash) { logprintf (LOG_VERBOSE, _("Removing %s.\n"), quote (name)); -- 2.7.3
wget-tests.tar.xz
Description: application/xz
pgprNP8iksA3W.pgp
Description: PGP signature
