Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve OGR CAD driver #172

Closed
wants to merge 8 commits into from
Closed

Improve OGR CAD driver #172

wants to merge 8 commits into from

Conversation

perob
Copy link

@perob perob commented Nov 30, 2016

Ticket #6739
Summary of changes:

  1. Add CRC validation to header variables section, classes section and for individual objects/entities
  2. Add more encoding for text recoding
  3. Avoid infinite loop when reading layer table

@perob perob changed the title Improve OGR CAD driver (#6739) Improve OGR CAD driver Nov 30, 2016
@@ -150,7 +150,7 @@ void RegisterOGRCAD()

CPLString CADRecode( const CPLString& sString, int CADEncoding )
{
const char* pszSource[44] = {
const char* pszSource[45] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just remove the explicit number ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure no problem

/* 44 ANSI_1258 */ "CP1258"
};
if( CADEncoding > 0 && CADEncoding < 45 )
if( CADEncoding > 0 && CADEncoding < 45 && CADEncoding != 4 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(pszSource) / sizeof(pszSource[0]) would be more robust than the hard coded 45

@rouault
Copy link
Member

rouault commented Nov 30, 2016

@BishopGIS I let you handle this

size_t nRecordsInSection = 0;

// read section data
pFileIO->Read( pabySectionContent, dSectionSize );
unsigned int dSectionBitSize = (dSectionSize * 8) - 128; // 8 + 8*7
pFileIO->Seek( -2L, CADFileIO::SeekOrigin::CUR );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already read 2 bytes upper. Not need to read it twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we do, because size is included in CRC validation.

unsigned int dSectionBitSize = (dSectionSize * 8) - 128; // 8 + 8*7
pFileIO->Seek( -2L, CADFileIO::SeekOrigin::CUR );
pFileIO->Read( pabySectionContent, 2 + dSectionSize );
unsigned int dSectionBitSize = (2 + dSectionSize - 2) * 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 + dSectionSize - 2 will translate by compiler to dSectionSize

Copy link
Author

@perob perob Dec 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section structure is awkward, so I left this as a reminder. Comment may be more appropriate.

pFileIO->Read( pabySectionContent, 2 + dSectionSize );
unsigned int dSectionBitSize = (2 + dSectionSize - 2) * 8;

size_t nBitOffsetFromStart = 16;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the magic number 16?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skip the size (short, 2 bytes). See comment above.

@BishopGIS
Copy link
Member

Merge to svn in r36752. Thanks @perob! This PR may be closed.

rouault pushed a commit that referenced this pull request Dec 9, 2016
…s. Add CRC validation to header variables section, classes section and for individual objects/entities. Add more encoding for text recoding. Avoid infinite loop when reading layer table

git-svn-id: https://svn.osgeo.org/gdal/trunk@36752 f0d54148-0727-0410-94bb-9a71ac55c965
@rouault
Copy link
Member

rouault commented Dec 9, 2016

@BishopGIS By the way, I see std::cerr uses in libopencad (some already existing, some new added). Would be good if it had an abstraction for error reporting, that could default to std:cerr for standalone libopencad, and that could be plugged to CPLError() when called from the OGR CAD driver.

@rouault rouault closed this Dec 9, 2016
@BishopGIS
Copy link
Member

@rouault good idea. I'll fix this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants