Fix memcpy size in pc_patch_wkb_set_int32

[elemoine]

Using -Wall and -O2 gcc 8.2.0 warns us about memcpy's that are out-of-bounds.

In file included from /usr/include/string.h:494,
                 from ../lib/pc_api.h:18,
                 from pc_pgsql.h:12,
                 from pc_pgsql.c:14:
In function ‘memcpy’,
    inlined from ‘pc_patch_wkb_set_int32’ at pc_pgsql.c:863:2,
    inlined from ‘pc_patch_to_geometry_wkb_envelope’ at pc_pgsql.c:918:8:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:34:10: warning: ‘__builtin_memcpy’ forming offset [5, 8] is out of the bounds [0, 4] of object ‘i’ with type ‘uint32_t’ {aka ‘unsigned int’} [-Warray-bounds]
   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

gcc is correct, there is a bug in pc_patch_wkb_set_int32. We should copy 4 bytes instead of 8 bytes.

[pblottiere]

@elemoine Nice, good catch!

Do you think that a unit test could be added to underline the underlying bugfix?

[elemoine]

Do you think that a unit test could be added to underline the underlying bugfix?
Merge state

We have unit tests for functions in lib only. If we wanted to unit-test the pc_patch_wkb_set_* functions we'd probably need to move them to lib. I think this should be done with a separate PR.

[pblottiere]

We have unit tests for functions in lib only. If we wanted to unit-test the pc_patch_wkb_set_* functions we'd probably need to move them to lib. I think this should be done with a separate PR.

Ok for doing this in another PR.

However, I'm wondering if we should move cunit tests directory in the root directory of pointcloud instead. This way, we have:

• pointcloud/lib
• pointcloud/pgsql
• pointcloud/cunit

@elemoine What do you think? But we'll deal with that later.

Thanks for the bugfix!

[elemoine]

pgsql includes PostgreSQL-related code, while the code in lib is PostgreSQL-agnostic. So for this particular case I think I'd just move the pc_patch_wkb_set_* functions to lib.

In general the pgsql directory should just include wrapping code, not logic. The logic should be in lib, so I think I'd leave the unit tests in lib.

[mbredif]

We have unit tests for functions in lib only. If we wanted to unit-test the pc_patch_wkb_set_* functions we'd probably need to move them to lib. I think this should be done with a separate PR.

if you recall, I tested that move a while back, eg in this commit, which fixed the memcpy issue while movibg the code and properly handling degenerate bounds :

LI3DS@788dc49

would you be interested in a rebased PR using this commit?

[elemoine]

@mbredif yep, that looks like a very good change! A PR would be great.