Make print methods in libnml/rcs/rcs_print.cc handle any size strings

[petterreinholdtsen]

Rewrite rcs_vprint(), rcs_print() and rcs_print_sys_error() to always print
full string and drop use of static fixed size temp string. Also make sure
trailing NUL is counted when copying strings into list of last errors.
Set output limit to 2^16 to avoid any malloc bombs.

This is the warning.

Compiling libnml/rcs/rcs_print.cc
In file included from /usr/include/string.h:495,
from libnml/rcs/rcs_print.cc:21:
In function ‘char* strncpy(char*, const char*, size_t)’,
inlined from ‘int rcs_vprint(const char*, __va_list_tag*, int)’ at libnml/rcs/rcs_print.cc:311:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:34: warning: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ >
106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
| ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[SebKuzminsky]

Thanks for improving the safety of the string handling here!

libnml is not realtime code: it's only used to communicate between non-realtime components, as shown here: http://linuxcnc.org/docs/devel/html/code/code-notes.html#_architecture_overview

Despite this, we try to avoid malloc when possible since it can cause long delays. These delays can show up for example in the GUIs or in Task, both of which can have negative effects on the behavior of the machine.

Can you rewrite this PR so it preserves the malloc-free behavior?

[petterreinholdtsen]

[Sebastian Kuzminsky]k ***@***.***> writes:

Despite this, we try to avoid malloc when possible since it can cause long delays. These delays can show up for example in the GUIs or in Task, both of which can have negative effects on the behavior of the machine.

This sound strange. Do you have a way to trigger such delays?

Can you rewrite this PR so it preserves the malloc-free behavior?

Not really possible, as it would reintroduce a statically declared array and thus place a limit to the string sizes again. I could get rid of the warning, but not make the print methods work in the generic case.

…

-- Happy hacking Petter Reinholdtsen

[SebKuzminsky]

Do you have a way to trigger such delays?

We don't have an automated test for this, but let's try a thought experiment. Consider a machine that's configured with swap, and whose RAM is nearly all in use. At this point, a malloc() and subsequent memory access can easily require a page-out to satisfy, which can take milliseconds and milliseconds.

For this reason we try to avoid dynamic memory allocation whenever possible in our realtime and "realtime-adjacent" code (e.g. GUIs, Task, IO, etc). I believe this is standard best practice, e.g.: https://wiki.linuxfoundation.org/realtime/documentation/howto/applications/memory#dynamic_memory_allocation_in_rt_threads

Not really possible, as it would reintroduce a statically declared array
and thus place a limit to the string sizes again. I could get rid of
the warning, but not make the print methods work in the generic case.

I think it's preferable to have a statically allocated string buffer, despite the limitation this implies for output string length.

[smoe]

I tend to concur that LinuxCNC needs to work in low-memory conditions and something "presumed harmless" like Thunderbird easily provokes such situations. This is why Thunderbird is no longer on my 8GB laptop. Is anything in LinuxCNC monitoring the available memory and inducing a soft stop when some reasonable limit was reached? If no such memory-watchdog yet exists, is that something that LinuxCNC should have?

[jepler]

libnml is linked only with non-realtime code so I don't see it as a priority to avoid memory allocations in it.

[SebKuzminsky]

libnml is linked only with non-realtime code so I don't see it as a priority to avoid memory allocations in it.

That's a fair point, however, latency in the non-realtime code a frequent source of problems, at least in the automated tests in the buildbot but also at least in theory in real-world use.

For example: a user presses a jog button in a GUI, the jog starts, the user releases the jog button, but latency in the GUI or in Task (e.g. in the NML that's connecting them) causes the jog to go on for a surprisingly long time.

This is why I advocate a realtime-only path for controls that command machine motion, such as jogging an axis (i.e. a physical jog wheel connected to motion, not to the GUI). But I think in practice a lot of us (me included!) like the convenience of jogging via the GUI, so avoiding latency in the GUI (and Task) is valuable IMO.

Unless there's a compelling argument why arbitrary-length debug log messages are super important, I prefer the previous, malloc-free code. I'd much prefer making the static message buffer much larger, if there are some big messages we can't currently print.

[petterreinholdtsen]

[Sebastian Kuzminsky]

Unless there's a compelling argument why arbitrary-length debug log messages are super important, I prefer the previous, malloc-free code. I'd much prefer making the static message buffer much larger, if there are some big messages we can't currently print.

How do you ensure that the rest of the code and library methods used avoid malloc()? It seem like a useless strightjacked to put on "our" code unless we can ensure that everyone elses code also avoid dynamic allocation of memory. After all, we only need one malloc() to mess up the timing, and one failing malloc() to crash the program. I do not know how the debug log message methods are use, nor how long messages they can encounter, but find it best to remove as many arbitrary limits from programs as possible, as they tend to, in my experience, to bring surprising or catastrofic results when least wanted. If we are serious about avoiding malloc(), we can override it in our code and provide a methd that always print an warning and return NULL (or just call abort()), to weed out these potential hazards from linuxcnc and its support libraries.

…

-- Happy hacking Petter Reinholdtsen

[rene-dev]

why not use std::string?

[SebKuzminsky]

why not use std::string?

If we decide we don't mind running malloc, then std::string is probably better than building our own dynamic strings from scratch.

[smoe]

How much memory are we talking about? We could have 10k reserved as a static variable and if anything longer is encountered then .... have an error message?

[rmu75]

my 2c: the fixed-size temporary string buffers (256 + 512 bytes) are allocated on the stack und could therefore also cause a pageout to swap. that could only be avoided with a per-thread mlocked statically allocated global temp buffer which seems a bit on the extreme side. I would go with std::string. Most GUIs are written in python, dynamic behaviour there on "the other end" of the log message is probably way worse than one dynamic allocation.

[rene-dev]

I had a go at this using variadic templates, and std::snprintf.
then I noticed most of the code isnt used out of libnml anyway, linuxcnc specific stuff really only uses it for normal printing.
so I suggest to change the linuxcnc stuff to normal printing, and remove all of rcs_print together with libnml when we ever get rid of libnml.

template<typename ... Args>
auto string_format(const std::string& fmt, Args... args)
{
    auto len = std::snprintf(nullptr, 0, fmt.c_str(), args...) + 1;
    auto buf = std::make_unique<std::string>(len, '\0');
    if( len <= 0 ){ return buf; }
    std::snprintf(buf->data(), len, fmt.c_str(), args...);
    return buf;
}