Skip to content

memcpy size seems erroneous for u_int32_t : 8 -> 4 - #96

Closed
mbredif wants to merge 1 commit into
pgpointcloud:masterfrom
LI3DS:patch-1
Closed

memcpy size seems erroneous for u_int32_t : 8 -> 4#96
mbredif wants to merge 1 commit into
pgpointcloud:masterfrom
LI3DS:patch-1

Conversation

@mbredif

Copy link
Copy Markdown
Contributor

(not tested)

@strk

strk commented Feb 18, 2016

Copy link
Copy Markdown
Member

Sounds good, but please add a test.

@mbredif

Copy link
Copy Markdown
ContributorAuthor

pc_patch_wkb_set_int32 is not defined in the header file, so I guess I cannot test it directly. It appears that it is only used in pc_patch_to_geometry_wkb_envelope where the "bug" does not matter much as int32s are written in sequence and followed by a pc_patch_wkb_set_double (l.976), so the writing is well defined. I do not know the exact spec of memcpy, but attempting to read 8 bytes from an 4-byte int does not sound good to me anyhow.

@strkstrk added the bug label Mar 1, 2016
@strk

strk commented Mar 1, 2016

Copy link
Copy Markdown
Member

reading 8 bytes from an 4-byte int is undefined behavior. the app would mostly read random 4 bytes from somewhere, which may or may not be assigned to the process (if it's not it'll be a segmentation fault).

Valgrind should be able to report such invalid read, so even if a test doesn't properly fail, it could be still useful to check it via Valgrind, and require it to run valgrind-clean (something that could be added to the travis check, btw).

mbredif added a commit to LI3DS/pointcloud that referenced this pull request Mar 7, 2016
…kb (in lib)
- added cunit testing
- fixing memcpy size 8 -> 4 (pgpointcloud#96)
@mbredif

Copy link
Copy Markdown
ContributorAuthor

While trying to get valgrind notice that issue, I ended up moving the envelope wkb construction code from the pgsql to the lib directory as a pc_bounds_to_wkb function, leaving only the serialization relevant code in pgsql.

The issue is that make installcheck is more of a black box than make check so that embedding it and PostgreSQL within valgrind is beyond my reach. I was able to move the code and provide a cunit test, but I could not get valgrind to notice the wrong size parameter.

@strk, do you mind if I package that within the same PR as the disambiguation discussed in #30 :

  • PC_Envelope(pcpatch) : wkb -> PC_Envelope_AsBinary(pcpatch) : wkb
  • geometry(pcpatch) : geometry -> PC_Envelope(pcpatch) : geometry

@mbredif

Copy link
Copy Markdown
ContributorAuthor

Superseded by #104.

@mbredifmbredif closed this Mar 8, 2016
@mbredif
mbredif deleted the patch-1 branch March 8, 2016 13:11
@mbredifmbredif mentioned this pull request Jan 31, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mbredif@strk