[issue #229] Prevent Buffer Overflows When Constructing Messages

[david-pace]

• increase buffer sizes
• change sprintf() calls to snprintf() calls introducing respective maximum size arguments
• remove #if / #endif blocks dealing with system names on Linux
• comment message informing the user that locking worked
• correct variable name and remove unused buffer name[80] in uucp_lock()

It would be great if someone could test this because I'm not a C(++) developer and don't have the toolchains installed.

[fwolter]

Thanks for fixing all these! You can test your changes with openHAB like the following:

Extract the JAR from the build artifact of your PR (https://github.com/NeuronRobotics/nrjavaserial/suites/7770122489/artifacts/326856992) to /usr/share/openhab/addons. After a few seconds, the new bundle should show up in the openHAB console (308 in my case):

openhab> bundle:list | grep nrj
254 │ Active │  80 │ 5.2.1.OH1              │ nrjavaserial
308 │ Active │  80 │ 5.2.1                  │ nrjavaserial

Then, you can disable the original bundle (5.2.1.OH1):

openhab> bundle:stop 254
openhab> bundle:list | grep nrj
254 │ Resolved │  80 │ 5.2.1.OH1              │ nrjavaserial
308 │ Active   │  80 │ 5.2.1                  │ nrjavaserial

There might be a restart of openHAB necessary.

[david-pace]

Thanks for the detailed description, @fwolter :+1: I managed to deploy the latest build into openHAB and to disable the OH bundle. It looked like this:

openhab> bundle:list | grep nrj
259 │ Resolved │  80 │ 5.2.1.OH1              │ nrjavaserial
303 │ Active   │  80 │ 5.2.1                  │ nrjavaserial

I also restarted the system and verified that the new bundle was still active. Then I tried to enable my Zigbee USB stick using the device name /dev/serial/by-id/usb-Silicon_Labs_BV_2010_10_01382705-if00-port0. Unfortunately I observe the same behavior as before: as soon as the nativeopen() method is called, the system crashes.

I'm quite sure that the Java Code can be exchanged by disabling a bundle and activating another bundle. Are you sure that this also works for the native code? Do you have any idea how I could verify that the new native code is actually running?

[fwolter]

I guess you have a point there. It might be necessary to rebuild the entire serial-transport feature to load the correct native code right from the start. https://github.com/openhab/openhab-core/blob/2e7fd9d72a256518684d3228a1bd5873a4ef331d/features/karaf/openhab-tp/src/main/feature/feature.xml#L214

[david-pace]

I managed to build a custom feature.xml file with version 3.4.0-SNAPSHOT, in which I referenced

<bundle>mvn:com.neuronrobotics/nrjavaserial/5.2.1</bundle>

instead of

<bundle>mvn:com.neuronrobotics/nrjavaserial/5.2.1.OH1</bundle>

I placed this file at /usr/share/openhab/runtime/system/org/openhab/distro/distro/3.4.0-SNAPSHOT/distro-3.4.0-SNAPSHOT-features.xml. Then I added this feature as a repository:

feature:repo-add mvn:org.openhab.distro/distro/3.4.0-SNAPSHOT/xml/features

Then I could add the openhab.tp-serial-rxtx feature:

feature:install openhab.tp-serial-rxtx/3.4.0-SNAPSHOT

Unfortunately this fails with:

Native Library /var/lib/openhab/tmp/libNRJavaSerial_openhab_0/libNRJavaSerial.so already loaded in another classloader
java.lang.ExceptionInInitializerError thrown while loading gnu.io.RXTXCommDriver
java.lang.NoClassDefFoundError: Could not initialize class gnu.io.RXTXCommDriver thrown while loading gnu.io.RXTXCommDriver
java.lang.NoClassDefFoundError: Could not initialize class gnu.io.RXTXCommDriver thrown while loading gnu.io.RXTXCommDriver

Any ideas how to proceed?

[fwolter]

You can replace /usr/share/openhab/runtime/system/com/neuronrobotics/nrjavaserial/5.2.1.OH1/nrjavaserial-5.2.1.OH1.jar by your JAR. I compiled a version (based on your PR) that also prints a message to stdout whenever a serial port is opened to show that the right version is taken: "RXTX Prerelease for testing Nov 20 2022". https://github.com/fwolter/nrjavaserial/releases/download/OH-2022-11-20/nrjavaserial-5.2.1.OH1.jar

You can view stdout by redirecting the karaf output to a file:

root@serial-raspi:/usr/share/openhab/runtime/bin# KARAF_REDIRECT=/tmp/out.txt ./start
start: Redirecting Karaf output to /tmp/out.txt

EDIT: Watch out, it has locking enabled. You could see some blocked serial ports after a crash or an unexpected disconnect.

[david-pace]

Thanks for preparing the JAR 👍 I successfully replaced the bundle. I did not get the output stream redirect activated with my Ubtuntu (systemd) service, but I could verify that the bundle was indeed replaced using the bundle:header command. The output contained

Bnd-LastModified = 1668947510617
Created-By = 11.0.11 (Ubuntu)

which is different from the original bundle. With this version I can confirm that it is now possible to use longer device names. I tested with

/dev/serial/by-id/usb-SHK_NANO_CUL_433-if00-port0
/dev/serial/by-id/usb-Silicon_Labs_BV_2010_10_01382705-if00-port0

and experienced no more crashes :+1: So from my side the tests are successful. Are there any other unit and/or integration tests that you guys can run or is this covered by the build already?

Please also have a look at the code again and resolve the open conversations if you are fine with my changes.

[MrDOS]

Thank you for confirming that your changes work, @david-pace. The only automated tests we have are squirrelled away in #182, and I'll eventually try to resuscitate those.

I tested with

/dev/serial/by-id/usb-SHK_NANO_CUL_433-if00-port0
/dev/serial/by-id/usb-Silicon_Labs_BV_2010_10_01382705-if00-port0

I meant to mention this sooner, but I suspect you could also easily test this by symlinking a serial device to a different path to give it a longer name. E.g., ln -s /dev/ttyS0 /tmp/this-is-really-dev-ttyS0-but-now-it-has-a-long-name.