Compiler errors, and potential problems

[hb020]

TL;DR

There are a certain number of issues with the AR488 code that can cause crashes: array size too small and trying to read a string from a NULL pointer. They are mostly (only?) in the prologix interpreter.

These issues also prevent the use of the code in other environments. The code will crash when compiled with picolibc (like for example via Zephyr). Picolibc has no protection against NULL pointer use, unlike the Arduino libc.

There are 3 ways we can go about this:

1. let it be
2. I repair that. I will make sure that there are no structural changes, all functions still have the same signature, and will be at the same place. There will just be a bit more protection inside of them. It won't be much effort for me now.
3. you repair that.

Tell me how you want to take that on.

Problem 1

Level: serious, can provoke problems.

My compiler complains about the size of an array:

/...../AR488_ComPorts.cpp: In function 'void printHexAscii(uint8_t)':
/...../AR488_ComPorts.cpp:128:12: error: 'sprintf' output 8 bytes into a destination of size 6
  128 |     sprintf(x,"%c [%02X]\n", byteval, byteval);
      |     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Proposition:

Problem 2.

Level: Serious

fndl_h uses uint8_t addrList[15] = {0}; But it can be filled up to 32 elements (not 31!).

So: please use 32, and adapt some more code, from

void fndl_h(char *params) {
  char *param;
  uint16_t addrval = 0;
  uint8_t addrList[15] = {0};
.....
  // Initialise arrays
  for (int i = 0; i < 15; i++) {
    addrList[i] = 0;
  }
.....
  if (j==0) {
    // Read address parameters into array
    while (j < 15) {

to

void fndl_h(char *params) {
  char *param;
  uint16_t addrval = 0;
  uint8_t addrList[32] = {0};
.....
  // Initialise arrays
  for (int i = 0; i < (int)sizeof(addrList); i++) {
    addrList[i] = 0;
  }
.....
  if (j==0) {
    // Read address parameters into array
    while (j < (int)sizeof(addrList)) {

Problem 3.

Level: serious. Partly depends how you look at it, how you use it, partly crashing

When compiled outside of Arduino, and therefore using others libc implementations, like picolibc, one encounters many serious problems. These are all due to the fact that the existing AR488 code does not always check for NULL pointers. Picolibc does NOT check for parameter validity in functions like strncasecmp or strtol or strlen (see here for example.).

But the code changes that would be needed in AR488 for that are everywhere. Some functions are protected, some are not, like send_h(), xdiag_h(), fndl_h().

And there are cases where the protection of strlen would not matter, since it would crash also on arduino:
Try this with param NULL: if (param[strlen(param)-1] == '?') isQuery = true; (in send_h())

and isNumber(NULL) = true. Not great.

Problem 4.

Level: minor.

My compiler complains about an unused parameter.

/...../AR488_main.cpp: In function 'void default_h(char*)':
/...../AR488_main.cpp:2214:22: error: unused parameter 'params' [-Werror=unused-parameter]
 2108
 | void default_h(char *params) {
 |                ~~~~~~^~~~~~

Proposition:

Problem 5.

Level: don't know yet.

see the following code snippet from AR488.ino, inside void id_h(char *params):

      if (strncasecmp(keyword, "name", 4)==0) {
        dataPort.println(gpibBus.cfg.sname);
        return;      
      } void addr_h(char *params);
      if (strncasecmp(keyword, "serial", 6)==0) {
        unsigned long int serialnum = gpibBus.cfg.serial;

No idea what that void addr_h(char *params); is doing there. Probably a copy/paste that went rogue.

Problem 6.

Level: buggy when outside of Arduino

GPIB_CFG_SIZE is off when compiled outside of Arduino: it is too small, thereby missing the last bytes in the struct.

It is likely that my compiler does not want to pack the struct at byte boundaries, making it bigger than how it is under Arduino.

When adding

	static_assert(sizeof(gpibBus.cfg) == GPIB_CFG_SIZE, "The size of the GPIB configuration is incorrect. Please check  AR488_GPIBbus.h, and adjust GPIB_CFG_SIZE accordingly.");

it becomes visible. No idea if the Arduino compiler supports that though.
There are other ways of coding it, avoiding the need for GPIB_CFG_SIZE: one can eliminate db and just pass sizeof() to the read/write functions.

[hb020]

edited the above text a bit (again), added a TL;DR with proposition

[Twilight-Logic]

hb020, thanks for taking the time to post this. Sorry its taken me a while to get back to you. Have had rather a lot on and trying to deal with one thing at a time!

Problem 1

Its curious that although the final result will always be 5 characters plus terminating null (character, space, two hex digits , newline, null), that the compiler seems to want 8? I have come across this before and just updated the size of the field as the compiler wanted, but I am not sure whether this can be caught "on the fly" so to speak, but the change you propose is an easy fix so I have no problem with implementing that.

Problem 2

For some reason, Prologix chose to implement only 15 address parameters for their trigger and serial poll functions. I guess they thought it unlikely that anyone would manually specify anywhere near that many addresses. The other possibility is that since they used an ATMega164 with just 16kb or flash and only 1kb of RAM, that they were being particularly careful regarding RAM usage. When I originally wrote the code I just followed their documentation. Anything over 15 addresses would simply be ignored and only the first 15 read into the buffer and processed. I am curious about your statement that it requires 32 rather than 31. No particular objection to that, but one of those addresses would be the controller address which you would not want to include in trigger or spoll, although I am not too certain about fndl? Presumably the controller does not need to find itself using fndl either? In any case, I see no reason why a 32-address buffer could not be implemented. A check for the controller address could be added as well, as is the case with spoll. I also take your point about using sizeof() where applicable rather than a fixed number, but one has to be careful as it does not always return the value expected.

Problem 3

I need to have a look at the functions you mention. strncasecmp, strtol and strlen are standard C functions so I don't see any particular problem with using them. However, boundary checks are important and I will have a look at some of the other things you mention.

Problem 4

If that solves the error message or warning than I am OK with that.

Problem 5

Agreed. Does look like a rogue paste error. I will remove it.

Problem 6

Not sure that I understand what's going on here yet either, but will investigate. If I had to guess, then it might be because of the size of the 'bool' type? All of the other variables have fixed sizes (uintXX_t) and char is also a single 8-bit value. However although bool might be a single 8-bit byte on an 8-bit platform, I suppose it could have a different size on a 16-bit or 32-bit platform. The other explanation is the one you mention, that the compiler does not want to pack at 8-bit byte boundaries.

It was written a very long time ago, but I believe that 'db' was only there as a workaround to provide a way to save the data as a single block to EEPROM. It is possible to save a struct, but I think there was some problem with calling sizeof() on it as it would return the size of the struct address instead. I think you are correct that the size of the config struct could be determined dynamically though the sizeof() function within that class. It just then needs a member function to access that value externally, which could then be passed to the EEPROM read or write functions. As you say, the GPIB_CFG_SIZE macro could be eliminated.

---

I haven't done much with the AR488 code over the last couple of months but will try and find some time to start working on these problems. I will update this issue as I do so, although it may not necessarily be in numerical order!

[Twilight-Logic]

Problems 4, 5 & 6 have been addressed in the new release 0.53.41.
Issues 1, 2 & 3 still to be addressed.

[Twilight-Logic]

Problem 1:
The issue was that the X type character in sprintf/snprintf that converts from decimal to hex represents an 'unsigned int' type, the size of which varies from platform to platform. Even though a fixed uint8_t was being passed to the function, the underlying data type used for processing the command will have been an unsigned int. This has now been addressed in version 0.53.42.

Problem2:
In version 0.53.42, the ++spoll, ++trg and ++fndl function have been updated to accept 32 address parameters (0-31). I haven't implemented a check for the controller's own address in ++fndl as perhaps ++fndl should report everything that it finds including itself.

Problem3:
Still to be resolved. It will likely take some time as it requires a review of the entire code base.

[hb020]

as perhaps ++fndl should report everything that it finds including itself.

That is indeed what I would prefer, especially since one can move the controller to another address, but maybe not what the "standard" says.

[Twilight-Logic]

Problem 3: I have updated the code to deal with those errors in the functions mentioned and a handful besides. Can't be sure whether I have caught everything though. I would be interested to know what board or MCU your are compiling for and what toolset you are using it its not the Arduino IDE? I have used cppcheck but have yet to experiment with Clang static analyzer, but the codebase has gotten quite big so could do with using some tools to find stuff like this.

[hb020]

I'm doing it in Zephyr on a STM32H563 on picolibc. If you never used Zephyr, it can be daunting to start with it.

I see that you did modify the code in a lot of places, but I still see some problems (ex: in getCmd).

To simplify, the biggest that I still see missing is: every time you do a strtok, check for NULL returns.

With picolibc, any string handling function (DB_PRINT, strlen, strcpy, strncpy, strncasecmp, strcasecmp, strtoul, strncat, atoi, atol,..) WILL crash when given a NULL as parameter.
And arithmetic with NULL pointers WILL also crash. (example : params = token + strlen(token) + 1;)