Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#2613 closed defect (fixed)

INGR causes a bus error on Solaris 64-bit with Sun Studio 11

Reported by: tclarke Owned by: ilucena
Priority: normal Milestone:
Component: GDAL_Raster Version: 1.5.2
Severity: major Keywords: ingr
Cc: warmerdam

Description

Solaris 10 sparc 64-bit Sun Studio 11 builds of the INGR driver cause a BUS exception due to an alignment issue in the INGR driver. I'm attaching a patch which fixes the crash.

Attachments (1)

ingr_fix.patch (407 bytes) - added by tclarke 10 years ago.
Patch which fixes the crash

Download all attachments as: .zip

Change History (9)

Changed 10 years ago by tclarke

Attachment: ingr_fix.patch added

Patch which fixes the crash

comment:1 Changed 10 years ago by warmerdam

Cc: warmerdam added
Component: defaultGDAL_Raster
Keywords: ingr added
Owner: changed from warmerdam to ilucena
Version: unspecified1.5.2

Ivan,

Can you review and apply as appropriate. We should likely take this back into the 1.5 branch too (for 1.5.4).

comment:2 Changed 10 years ago by Even Rouault

If I can share my opinion, I think the proposed fix is only a workaround that prevents unaligned memory access but that's not the correct fix. I'd bet it will certainly fail to read any valid INGR dataset.

#pragma pack(8) will change the size of structures like the header (INGR_HeaderOne, INGR_HeaderTwoA, ...) which are in fact the exact layouts of the header on file when #pragma pack(1) is set.

The current code of the INGR driver should be changed to not directly fread in a single time the structure and access their fields. It should rather be something like :

VSIFReadL(&pHeaderOne->WordToFollow?, 1, sizeof(short), fp); VSIFReadL(&pHeaderOne->DataTypeCode?, 1, sizeof(short), fp); etc...

Same thing for the write part of course. Not a one-liner kind of fix obviously.

In a more general way, we should ban the use #pragma pack for any portable code on architectures not supported unaligned access. A quick scan of the gdal source code shows another use in the MSGN driver (in msg_basic_types.h).

comment:3 Changed 10 years ago by warmerdam

I would generally concur with Even's analysis. Depending on structure packing to match layouts of stuff from disk is very risky, and impedes portability.

comment:4 Changed 10 years ago by ilucena

I completely agree. I should have loaded the records it on a buffer and used memcpy() field by field. That also would lead to a better solution to take care of the LSB conversions. The question now is should I do it now since there some other tests to be done on RLE compression or should I apply the workaround and let those change for a later time. Be aware that are a lot of optionals sub-formats [1] supported by this driver and the test are going to be very time consuming.

[1] - Tile and untiled: RLE, RLE Color Table, RLE RGB, Adaptative RGB, CCITT 4, JPEG, Continuous Tone and the regular flat pixel blocks.

comment:5 Changed 10 years ago by Even Rouault

Workaround patch commited in r15529.

comment:6 Changed 10 years ago by ilucena

I have been working on a solution to eliminate the pragma declaration. So far, the tests are proven the changes to be successful and I will commit it shortly. Thanks for the help and advices.

comment:7 Changed 10 years ago by ilucena

Resolution: fixed
Status: newclosed

Revision 15559 applied to trunk.

Loads and unloads struct to buffer and buffer to struct:

http://trac.osgeo.org/gdal/browser/trunk/gdal/frmts/ingr/IngrTypes.cpp#L1081

based on macro:

http://trac.osgeo.org/gdal/browser/trunk/gdal/frmts/ingr/IngrTypes.h#L539

comment:8 Changed 10 years ago by Even Rouault

Fix for compilation on big-endian host commited in r15563

Note: See TracTickets for help on using tickets.