CAPS library v5.1 - bug suspected

[Sugarbox]

Hello,

As I explained previously in the wrong forum, I suspected a bug on the library.

Here it is (people who don't stand technical information, please don't read what is following !)

On the source file CapsImage.cpp of the CapsImage folder, line 404, I read :

Code: Select all

PUBYTE trackbuf = pti->trackdata[rev];
int tracklen = allrev ? pti->rawlen : pti->tracksize[rev];

If I understand correctly what's all here, we try to put on the local variable tracklen the max size of the buffer to read.
Which is pti->tracksize[rev] for multiple rev, and meant to be rawlen if "allrev" is different from 0.

But....

Following, we have (line 420) :

Code: Select all

while (bofs < tracklen) {
    // get actual byte from track buffer
    uint8_t bval = trackbuf[bofs];

Which would lead to return out of buffer values in the following case (which I encountered) :
- allrev = 1
- rev = 1
In these case, the index buffer of the trackbuf would be : Index of first rev + bofs (which can be up to rawlen ) > rawlen.

I expected either tracklen to be equal to pti->tracksize[rev], OR trackbuf to be equal to pti->trackdata[0] in case of allrev...

I hope this make sense

This can lead to misunderstand of values (if lucky with read access of the buffer), or even some exceptions.
With the following correction on line 405 :

Code: Select all

int tracklen = pti->tracksize[rev];

It seems to work better.

Thomas

[IFW]

That fix is incorrect, because it seems to do what you want to achieve but it really does not

You must not read a revolution that does not exist - naturally, that would lead to errors.
Could you describe exactly how do you get rev == 1 if allrev is 1 as well? In this case rev should always be 0.

[IFW]

Would be great if you could post a file and exact details of how to repro the case you encountered.

[Sugarbox]

Well....
allrev = 1 (allrev is used as a boolean) because of

Code: Select all

int allrev = (pti->trackcnt == pti->rawtrackcnt);

rawtrackcnt = 5 because of :

Code: Select all

int maxrev = min(CAPS_MTRS, wh->trkcnt);
	pti->rawtrackcnt = maxrev;

AND wh->trkcnt = 5.

pti->trackcnt is computed as :

Code: Select all

// generate 1 revolution of data as default, 5 for analyser
	int trackcnt = (di.flag & DI_LOCK_ANA) ? 5 : 1;

	// 5 revolutions if track update is not used
	if (!(di.flag & DI_LOCK_UPDATEFD))
		trackcnt = 5;

	// the number of revolutions can't be more than the number of track revolutions decoded
	if (trackcnt > maxrev)
		trackcnt = maxrev;

	// write track info
	pti->trackcnt = trackcnt;

So, trackcnt = 5 because :
- flag does not contain the DI_LOCK_UPDATEFD bit.
- maxrev = 5

To have a reproductible case :

use this little piece of code :

Code: Select all

// TstCAPS.cpp : définit le point d'entrée pour l'application console.
//

#include <Stdint.h>
#include <string.h>
#include "CapsLibAll.h"


int _tmain(int argc, char* argv[])
{
   SDWORD image = CAPSAddImage ();

   if (image  < 0 )
   {
      return -3;
   }
   
   // Convertion from TCHAR to PCHAR
   if ( CAPSLockImage (image , "Strider.raw" ) == imgeOk )
   {
      CapsImageInfo imgInfo;
      memset (&imgInfo, 0 , sizeof (imgInfo));
      if (CAPSGetImageInfo (&imgInfo, image ) == imgeOk)
      {
         // For each head
         int nbSide = imgInfo.maxhead - imgInfo.minhead + 1;
         for (int head = 0; head < nbSide; head++)
         {
            // For each tracks
            for (int track = imgInfo.mincylinder; track <= imgInfo.maxcylinder; track++)
            {
               // Get track info
               CapsTrackInfo capsTrackInfo;

               memset (&capsTrackInfo, 0 , sizeof (capsTrackInfo));
               if ( CAPSLockTrack (&capsTrackInfo, image, track, head, DI_LOCK_TRKBIT/*DI_LOCK_INDEX*/) == imgeOk)
               {
                  int nbRevolutions = capsTrackInfo.trackcnt;
                  if (nbRevolutions > 0)
                  {
                     // For each revolution of this track
                     for (int rev = 0; rev < nbRevolutions; rev++)
                     {
                        // Do something smart here
                     }
                  }
                  CAPSUnlockTrack (image, track, head) ;
               }
            }
         }
      }

      CAPSUnlockImage (image);
      CAPSRemImage (image);
   }
   return 0;
}

- Use the attached "Strider.raw" ct-raw file
- Link with the latest recompiled librarie

And put some breakpoints on the line described in the pervious message.

Last note (if usefull ?) : I use the DI_LOCK_TRKBIT flag bit.

[IFW]

Thanks, I'll take a look

[Sugarbox]

Hello,

Did you find anything on it ?

Regards,

Thomas

[IFW]

Haven't had time yet as working on the next DTC release.
btw: as a work around, since you want to iterate all revolutions anyway, you can simply ask the library to return a single revolution only by using DI_LOCK_UPDATEFD - which is pretty much what any other software does.

[Sugarbox]

Thanks a lot !

If I set the revolution befor the lockTrack function, I can even read weak sectors.
Untill better, it will be enough.

[IFW]

Yes, CAPSGetInfo & CAPSSetRevolution are very useful for iterating the revolutions sampled. Actually your code is the only one that did not use single revolution retrieval, so the problem was well hidden...

[Sugarbox]

I think it is because of my paradigm : As I already coded the whole FDC, the only data I want to get from these files are the bitstreams.