search

LibVNC / libvncserver

LibVNCServer/LibVNCClient are cross-platform C libraries that allow you to easily implement VNC server or client functionality in your program.
GNU General Public License v2.0
1.11k stars 484 forks source link

Potential ABI breakage vs 0.9.13 #507

Closed aconverse closed 2 years ago

aconverse commented 2 years ago

If you'd like to put out an incentive for fixing this bug, you can do so at https://issuehunt.io/r/LibVNC/libvncserver

Describe the bug Please disregard this bug if the current code in the master branch (and the next release whenever that may be) is not intended to be ABI compatible with the 0.9.13 release. This would imply that linux distros cannot update libvncserver without rebuilding all consuming applications and is often accompanied with a soname bump.

It appears that 26bf37494a8ca5deaaa79d74cdda2da3af318e06 #468 adds a struct member setXCutTextUTF8 in the middle of struct _rfbScreenInfo. This causes computed offsets of all subsequent members to be shifted by sizeof(void*). This causes calling code to not have the same understanding of struct offsets and load the wrong values from memory for these fields.

To Reproduce

  1. Build libvncserver 0.9.13 with LIBVNCSERVER_HAVE_LIBZ
  2. Build a simple program that has a custom newClientHook.
  3. Observe that custom newClientHook is called.
  4. Update libvncserver to a04fed5377fda8eff818e0b62422c26ba43427ae
  5. Rebuild libvncserver
  6. Do NOT rebuild calling program
  7. Observe that default newClientHook is called.

This can be observed with in less integrated ways with the offsetof(rfbScreenInfo, newClientHook) and with pahole -C _rfbScreenInfo /path/to/libvncserver.so.0.9.13 when built with debug info.

Expected Behavior The program still works / pahole struct representations are compatible

Logs/Backtraces Attached is a diff of pahole -C _rfbScreenInfo /path/to/libvncserver.so.0.9.13 with the release and master.

libvnserver._rfbScreenInfo.diff.txt

Your environment (please complete the following information):

Additional context The most straightforward workaround would be to move the new field to the end of the struct.

bk138 commented 2 years ago

Thanks for the report! You mean https://github.com/LibVNC/libvncserver/commit/26bf37494a8ca5deaaa79d74cdda2da3af318e06#diff-682a84ead83abbcefd0862a81a8331d6017009c2fb339e7475063321f25d1b7dR298-R300 ? Yes, that seems to have slipped under the radar, thanks for spotting! Do you want to create a PR fixing this?