Ticket #136 (closed defect: fixed)

Opened 10 months ago

Last modified 8 months ago

Problem overwritting Reader's dataoffset due to VLR

Reported by: mrosen Owned by: hobu
Priority: major Milestone: 1.2.1
Component: General Version: 1.2
Keywords: Cc:
LAS Format Version: Not Applicable

Description

I recently tracked down a problem to what I believe is a failure in the way LASReader manages it's data offset pointer.

LASReader::Init first reads the header common fields and sets the data offset appropriately. Then it tells it's impl to ReadVLR(m_header). That sounds right but has the side effect of updating the header's data offset. While this bookkeeping is necessary for a Writer, for a Reader this behavior is wrong since it is possible that the point data will not immedately follow the VLRs. Indeed the 1.0 spec advises writers "it is recommended that additional space be allocated to the variable length header space, so that additional variable length data can be added after the initial data is written."

In any case, no harm is really done yet since the Reader still has the original offset it read when it first parsed the header. However, the last thing that Init does is to reset the Header and this flushes the updated data offset from the header to the reader where it potentially (and in some cases, actually) corrupts the reading of point data.

void LASReader::Init()
{    
    bool ret = m_pimpl->ReadHeader(m_header);

    if (!ret)
        throw std::runtime_error("public header block reading failure");
        
    ret = m_pimpl->ReadVLR(m_header);
    if (!ret)
        throw std::runtime_error("public vlr header block reading failure");
    
    m_pimpl->ReadGeoreference(m_header);
    // inappropriate for a reader? m_pimpl->Reset(m_header);
}

Change History

Changed 8 months ago by hobu

  • milestone set to 1.2.1

Mike,

I just saw this. Do you remember if you have a file that demonstrates it? I agree this is wonky, and I clearly didn't expect that there would be space between the end of the VLRs and the beginning of the data.

Howard

Changed 8 months ago by mrosen

No worries. Thanks for keeping up on this.

Our internal report lists several files, most of which are large enough to be inconvenient and none of which has a known owner (I'm sure we just downloaded them from somewhere on the internet when we first started the project).

However, one of them appears here http://liblas.org/samples/Srsota000011.las

We had difficulties decoding this file which we attributed to this issue.

I believe the offending LAS writer is is the one used by Merrick's MARS software.

If you are unable to reproduce the failure with this file, please let me know.

msr

Changed 8 months ago by hobu

I think I understand the problem now. I was using the DataOffset? of the header as the same thing as the location of the end of the VLRs. AddVLR, DeleteVLR, and ReadVLR all were updating the header's DataOffset?. As you said, for writing, this is perfectly fine. For reading, it is not.

The problem lives in the VLR code in LASHeader in my opinion, not the Reset function. I was probably flushing that as an attempt to cope with the dataoffset changing.

I need to think about this some more, but I suspect I will change the VLR code to do its own accounting of where the end of the VLRs currently resides and set that as an extra private variable on the header.

Also, the file you linked had no problems, but http://liblas.org/samples/USACE_Merrick_lots_of_VLRs_with_classes.las readily demonstrates the issues. I will try to make a trimmed version of this file for the test suite.

Changed 8 months ago by mrosen

Glad you were able to repro the issue. The internal bug report seems like it was dealing with two issues.

Thanks for dealing with this.

msr

Changed 8 months ago by hobu

See  http://lists.osgeo.org/pipermail/liblas-devel/2009-July/000568.html for more detail on this about backporting this to 1.2. r1325 fixes this for trunk.

Changed 8 months ago by hobu

  • status changed from new to closed
  • resolution set to fixed

Backported to 1.2 branch in r1327

Note: See TracTickets for help on using tickets.