Giter VIP home page Giter VIP logo

Comments (13)

scaprile avatar scaprile commented on August 21, 2024

I've had no problem using this library, which is also true for many other developers.
What is your issue, exactly ?

from paho.mqtt.embedded-c.

begincalendar avatar begincalendar commented on August 21, 2024

Mainly the following:

  • Integrating with pre-existing code that uses fixed-width types may result in unexpected/undesirable overflows and/or undefined behaviour (e.g. int32_t being passed to this library as int on a platform where int is only 16-bits wide).
  • More memory may be consumed than needed.
    E.g. ANSI C guarantees that int will be at least 16-bits wide, Let's say on platform X, int is 32-bits wide, but this library only needs the 16-bit width (in context Y). If the library had used int16_t (in context Y), there would be no wastage.
  • While ANSI C provides some guarantees for numeric type widths, they are not as concrete and controlled as fixed-width integers. Control is relatively more important in the embedded world (than say in the web dev world).
  • I would argue that code written with fixed-width types is easier to read/review.

This is not to single out embedded Paho, I think all embedded C/C++ code should use fixed-width types where feasible.

from paho.mqtt.embedded-c.

begincalendar avatar begincalendar commented on August 21, 2024

@scaprile I think we might have discovered at least one bug relating to this topic.

While the C standard guarantees that int is at least 16-bits wide, it appears as though the remaining length decoding function in this library assumes that int is at least 32-bits wide.
On the platform we are using, int is only 16-bits wide.

from paho.mqtt.embedded-c.

scaprile avatar scaprile commented on August 21, 2024

Yes, either both MQTTPacket_decode() and its child MQTTPacket_decodenb() (through the definition of struct MQTTtransport) assume int is at least 32-bits wide.
In fact, now that you mention it, this assumption is also held (at least) in MQTTPacket_len(), as it assumes both its parameter and returned value are at least 32-bits wide.
I can also assume that their callers carry the same assumption so this is a library-wide issue.

This issue seems to arise when packets are longer than 16383 bytes; and this is usually not common on 16-bit platforms. I'm curious to know if you are actually having an issue.

from paho.mqtt.embedded-c.

begincalendar avatar begincalendar commented on August 21, 2024

I'm curious to know if you are actually having an issue.

We are working on a proof of concept and the bug came up as part of our internal review of this library.

This issue seems to arise when packets are longer than 16383 bytes; and this is usually not common on 16-bit platforms.

For the proof of concept this will ring true for us, but for the long run we can't rely on chance to avoid the undefined behaviour of signed integer overflow (e.g. with multiple-pass streaming of a large enough amount of data).

from paho.mqtt.embedded-c.

scaprile avatar scaprile commented on August 21, 2024

I guess you don't have 16K+ RAM available to hold 16K MQTT packets on your 16-bit device, so you most likely will need to discard these packets anyway.
A packet with a 3-byte length field will overflow your int so I would catch this at MQTTPacket_decode() and MQTTPacket_decodenb() by setting

#define MAX_NO_OF_REMAINING_LENGTH_BYTES 2

Let's see what the maintainer thinks of this issue.

from paho.mqtt.embedded-c.

begincalendar avatar begincalendar commented on August 21, 2024

I guess you don't have 16K+ RAM available to hold 16K MQTT packets on your 16-bit device, so you most likely will need to discard these packets anyway.

That's correct.
We don't have 16K+ RAM available, but in the long run we would need to provide the ability to stream that amount of data via MQTT.
Given that this library (and I'd personally argue the MQTT protocol itself) assume that the length of an MQTT control packet is known up-front or that a control packet can always fit in memory, we would need to develop our own streaming APIs to suit our memory constraints.

A packet with a 3-byte length field will overflow your int so I would catch this at MQTTPacket_decode() and MQTTPacket_decodenb() by setting...

Thanks for the tip.

Let's see what the maintainer thinks of this issue.

That was my intention behind raising this decoding issue. While before it was more about a matter of opinion, now there is at least one concrete example of how fixed-width integer types would prevent bugs across any compatible platform.

from paho.mqtt.embedded-c.

OlegHahm avatar OlegHahm commented on August 21, 2024

Maybe #238 is of any help for you.

from paho.mqtt.embedded-c.

OlegHahm avatar OlegHahm commented on August 21, 2024

Btw the current code base don't even compile for a 16 bit platform.

from paho.mqtt.embedded-c.

scaprile avatar scaprile commented on August 21, 2024

Btw the current code base don't even compile for a 16 bit platform.

Perhaps you should define "compile" and "16-bit platform", this is a fairly mature project and has been developed when C89 dominated the embedded C world. Many sections assume 32-bit ints, though.

wrt your ssize_t idea, it might work in your platform and not on others, where the code assumes a 32-bit int and you have a 16-bit size_t.

Anyway, the maintainer will decide eventually.

from paho.mqtt.embedded-c.

OlegHahm avatar OlegHahm commented on August 21, 2024

Btw the current code base don't even compile for a 16 bit platform.

Perhaps you should define "compile" and "16-bit platform"

The definition of "compile" is very easy: take a C compiler and try to translate the source code into binaries. The term 16-bit platform is also very well defined: a computer architecture where the width of an integer or address has a width of 16 bit. If you try to take a compiler for such a platform the comparison in https://github.com/eclipse/paho.mqtt.embedded-c/blob/master/MQTTPacket/src/MQTTPacket.c#L93 will fail.

this is a fairly mature project and has been developed when C89 dominated the embedded C world.

Interesting, given the fact that the first MQTT specification has only been released in 1999. Even more interesting that back in these days 32 bit architectures were hardly available for microcontrollers.

wrt your ssize_t idea, it might work in your platform and not on others, where the code assumes a 32-bit int and you have a 16-bit size_t.

That's not correct. size_t is defined to always be big enough to store the "maximum size of a theoretically possible object of any type" according to ANSI C99 (compare https://en.cppreference.com/w/c/types/size_t or https://www.gnu.org/software/libc/manual/html_node/Important-Data-Types.html).

Anyway, the maintainer will decide eventually.

I'm under the impression there are no maintainers for this project any more given the fact that no pull request has been properly reviewed in the last couple of years - let alone be merged.

from paho.mqtt.embedded-c.

scaprile avatar scaprile commented on August 21, 2024

Oh, I do love irony and sarcasm too. Unfortunately, I can't afford the time. Since you do know for sure, you'll have no issues.
You can easily find the maintainer blog and ask him.

from paho.mqtt.embedded-c.

OlegHahm avatar OlegHahm commented on August 21, 2024

Maybe you'll find the time if you refrain from wasting it for passive-aggresive and non-helpful comments. Good luck!

from paho.mqtt.embedded-c.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.