libparanoia reads incorrect track with full paranoia, correct track with -Z and -Y

[RecursiveForest]

This is a duplicate of a bug report I made an savannah. I don't know where the preferable place to report bugs for this project is, and wanted to get as wide coverage as possible.

https://savannah.gnu.org/bugs/index.php?49831

[rocky]

Thanks for the report. Robert Kausch is looking at it and he's the best person to be doing so.

[zacklb]

I ran a bisect on the Xiph.org cdparanoia repository, ripping track 5 of the Roberts album, and found that this particular bug was introduced with the following revision:

$ git bisect good
6a89afab34f3319f926bd217dfd8d68737ac80df is the first bad commit
commit 6a89afab34f3319f926bd217dfd8d68737ac80df
Author: xiphmont <xiphmont@0101bb08-14d6-0310-b084-bc0e0c8e3800>
Date:   Thu Sep 11 20:09:40 2008 +0000

    New cache modelling in place and functioning.
    
    
    
    git-svn-id: https://svn.xiph.org/trunk/cdparanoia@15298 0101bb08-14d6-0310-b084-bc0e0c8e3800

:100644 100644 959b960859991419aaf622f745bbaa2e935fc150 ba03d995ddc2f0a0797d11543e4407b509d4fa31 M	cachetest.c
:100644 100644 97092c5e1bf6bfe877aedb0097fdb71bfd746efe cb625cc5bc540bb17851fa2d53e862f1201273b2 M	cdparanoia.1
:040000 040000 ed42a53c170c99a6728c8bc8f060b11361e702f3 49be449011a46647640db959e66584a61fc83f8a M	interface
:100644 100644 40941e7dc095af144a580d2d1ddccf3241d3977b f4ac812cdc7b2df3cff562c9d455abf9b4a88ea1 M	main.c
:040000 040000 366d21c0493ccd562792fa3a4e0798a301b8ee17 9b2ff5bff68774394d67940f3cc05bc5cf7f88a7 M	paranoia

[rocky]

Only just now have I had the time to look at this. In a branch I've applied both patches as branches pending Robert's decision as to which to go with.

It would be good to get in place a test for this. This is in neither patch, but I do see some references to it in the cd-paranoia bug report.

Do you think you could work up a test for this? Thanks.

[rocky]

The smallest offset patch is is failing: https://circleci.com/gh/rocky/libcdio-paranoia/15

The longest match patch however passes CI.

Note that both patches could not be applied without modification. They were probably made of the cdparanoia source, not the libcdio-paranoia source.

[ghost]

Hi, original author of the patches here. The failing test was because of a small error when applying the patch. I fixed this in #7.

As far as I know, there is no test yet that exposes this bug. I'll try and work something up.

[rocky]

@a10footsquirrel many thanks for the change and thanks in advance for working up a test.

[enzo1982]

Hi all! I apologize for not taking a closer look at this earlier. I did not have time to set up a test environment to reproduce this, but took the time today after your mails. I prefer the smallest offset patch. However, I think there are some minor issues with it: 1. I think a match might be found at offset -1. In this case, -1 should not be used to signal no match found; it would then go undetected. A better signal value might be LONG_MAX. 2. Before calling the callback with PARANOIA_CB_FIXUP_EDGE, it should check for min_offset instead of offset (the result will be the same, but it would be easier to read). BR, Robert

…

---- Robert Kausch robert.kausch@freac.org Am 05.02.2017 um 04:19 schrieb R. Bernstein:

Just now have I had a time to look at this. In a branch I've applied both patches as branches pending Robert's decision as to which to go with. It would be good to get in place a test for this. This is in neither patch, but I do see some references to it in the cd-paranoia bug report. Do you think you could work up a test for this? Thanks. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHb36TGGswvL0bvqY9Gg0E1H72MvRUsUks5rZT-sgaJpZM4LLK1e>.

[ghost]

Created another pull request for the changes suggested by @enzo1982 (#8).

Still working on the test case, it's a bit harder than expected because the bug really depends on the matching runs starting and ending around specific byte locations. (And I can't work on it that much because of personal life stuff having a higher priority.)

[ghost]

Hi, sorry that it took a while, but I created a pull request with a test case that fails on master and succeeds on both branches that fix the bug.

[ArchangeGabriel]

So, now that #7, #8, #9 and #10 are merged, this should be closed right?

Congrats to @a10footsquirrel for fixing this many years old bug! :)

[rocky]

I think so, but I usually let the reporter of an issue verify and close. But yes, thanks are due to @a10footsquirrel for fixing this many-years-old bug.

(A pity it isn't yet cd paranoia from which this derived, but maybe now this is here that might aid adding it there.)

[ghost]

Thanks, it was my pleasure. Any chance for an official release for this bug fix? I think the whipper (and other projects) will appreciate an updated version in the official repositories of most distributions as soon as possible.

[rocky]

Don't know when I'll have time, but I'll try to make a new release when I'm able.

[rocky]

Pushed libcdio-paranoia-10.2+0.94+1.tar.gz to ftp.gnu.org and tagged release-10.2+0.94+1

I am not sure this change will get reflected in whipper since it uses the stock cdparanoia

[ArchangeGabriel]

@rocky Thanks for the release, we will update whipper to libcdio-paranoia soon whipper-team/whipper#87.