Potential buffer overflow

[BsAtHome]

There is a possible buffer overflow by using strncat(), even though it only appends one character at a time. The target buffer is a static on-stack array (initialized with zeros). It should be a NUL-terminated string at all times. However, if the buffer is full and the NUL-terminator is positioned as last character in the array, then a call to strncat, appending one character, will overwrite the string termination and exhaust the buffer space.

Other places have already be replaced by rtapi "safe" api calls. The following diff fixes the problem and makes use of that api. It may not be as fast, but it is secure.

diff --git a/src/emc/rs274ngc/interp_remap.cc b/src/emc/rs274ngc/interp_remap.cc
index 3e4069e0e8..08a36deecc 100644
--- a/src/emc/rs274ngc/interp_remap.cc
+++ b/src/emc/rs274ngc/interp_remap.cc
@@ -327,8 +327,8 @@ int Interp::add_parameters(setup_pointer settings,
     }
     while (*s) {
        errored = true;
-       char c  = toupper(*s);
-       strncat(tail,&c,1);
+       char c[2]  = { (char)toupper(*s), 0 };
+       rtapi_strxcat(tail, c);
        if (*(s+1)) rtapi_strxcat(tail,",");
        s++;
     }

[rmu75]

interp is not running in any realtime context, so that could all be std::string ;-)

[BsAtHome]

interp is not running in any realtime context, so that could all be std::string ;-)

Yes, but that would mean rewriting a lot (really a lot) of code. That would be a project.

This fix is just a patch to stop the leaking. If someone feels up to it, please feel free to refactor the boat so that it never can leak again ;-)

[smoe]

Can we agree that the original code is ugly? What is it doing? Separating all characters in a string by commas? I presume the compiler will take care about the redundant errored assignments, but I wish this would be reimplemented.

[BsAtHome]

It is really ugly, yes. I already mentioned, doing a refactor would be a real (large) project.

But the question is, do you leave an known serious error in the code waiting for it to explode in your face? Or do you, as a bare minimum, patch-fix the error and put refactoring on the todo-list if you are not capable of doing the refactoring immediately?

[rmu75]

The original code is ugly. The potential for overflow isn't that large to begin with, tail is string buf length 255, missing is len 30, so copying parts of missing into tail can't overflow. Nonetheless usage of strncat is a bug. I don't like this solution, replacing those char[]s with std:: shouldn't involve too many changes. I will try to come up with a different fix later today.

[smoe]

The problem of the original code starts with line

linuxcnc/src/emc/rs274ngc/interp_remap.cc

Line 325 in 1ed894e

if (*s) {

, i.e. the if that is already testing on *s. Errored should be set there and the while become part of the diff, preferably becoming a do .. while loop on *(s+1).
The code is about preparing the error message, and the ESR macro (

linuxcnc/src/emc/rs274ngc/interp_internal.hh

Line 873 in 1ed894e

#define ERS(fmt, ...) \

) returns with an error. Have no idea about what the effect is, but somehow I expect/hope the application to end soon after - and if it is with a memory error :-)
In line

linuxcnc/src/emc/rs274ngc/interp_remap.cc

Line 240 in 1ed894e

std::fill(tail, std::end(tail), 0);

the tail[] is filled with 0s. So if LINELENGTH is not exceeded (which nobody checks) then the string is 0 terminated, taking away the initial motivation for this patch?
If you have the energy, my suggestion would be to fill tail directly, characterwise, without the rtapi/std:: functions and introduce a check on LINELENGTH at the same time. Otherwise, maybe just leave a FIXME with a comment that because of the 0-fill the compiler gives a false positive warning.

[BsAtHome]

... I will try to come up with a different fix later today.

I will yield this problem to you ;-)