Hard to chase (but reproducible) segfault in openni_camera/wrapper code

[jkammerl]

Mail from Peter Soetens to pcl-developers:

Hi,

Setup is an Xtion Live, OpenNI 1.5.4.0 and Sensor 5.1.0.41, with our
callbacks added to the PCL openni grabber, and capturing images+depth.
Segfault occurs before our callback is called, and valgrind points
to nothing interesting (we're not corrupting anything).

So it happens only once every so many hours, but the core file always
points to the same line, a null pointer reference here:
#0 openni_wrapper::ImageYUV422::fillRGB (this=,

width=, height=480,
rgb_buffer=0x7f31f412cdb0
"V^bW_cTcTcW^eX_fTcVbeUadVbeV^bXdYaeYaeVbeVbeYaeYaeYaeYaeX_fX_fUadTcQ]O[^RY`RY`RX]RX]SY^SY^R[ZR[ZRZ^RZ^S[_S[TZaTZaTZ_TZ_SZaSZaS[T]\SZaU\cU]aU]aU]aU]aU\cU\cU]aU]aV^bV^bU]aV^bU\cV]dU\cV]dU\cW^eT"...,
rgb_line_step=) at
/home/intermodalics/ros/pcl/io/src/openni_camera/openni_image_yuv_422.cpp:91

There are only two reasons why yuv_buffer can possibly be null: a bug
in the OpenNI driver code, or a bug in PCL's image copying mechanism.
I'm suspecting it's the last (for now)

It appears that in openni_device.cpp (line 879), the image is copied like this:

image_data->CopyFrom (image_md);

There are two serious issues in this line:

1. the return value is not checked, but the alloc may fail, in which
   case image_data is not a copy but something 'undefined'.
2. CopyFrom makes a 'deep' copy, but only if you access it through
   'WritableData()' , not through 'Data()'. Incidentally, the openni
   wrapper code consistently uses 'Data()'. So the CopyFrom() has no real
   effect, and we keep accessing the original data pointer.

IMHO the real bug is in OpenNI itself, where it should return the copy
of the data in Data() after a CopyFrom(), but it certainly doesn't...

The attached patch for your review addresses both issues. It's hard to
say that this is it, but it's certainly fishy. I haven't been able to
reproduce the segfault in Debug builds, maybe it jumps tomorrow...

Another wild theory is that this CopyFrom is completely unneccesary if
image_md is never updated outside the ImageDataThreadFunction()....and
that the bug is indeed elsewhere.

I'm now also seeing that DepthMetaData::operator[] uses 'Data()' as
well when reading/converting depth images. So in any case, there's
more work to do to figure this out and consistently fix all 3 streams.

Peter

Patch applied here:
#104