1

Sorry for novice question in Arduino, but I'm trying to figure out why res is keeping its value when up_cmd0 is being called again.

for example - if num=8 in first run and the result is :up_cmd0_res: 8, and second run num=12, the result is: up_cmd0_res: 812

void up_cmd0(){
  char num[8];
  char *res = "up_cmd0_res:";
  itoa(counter2, num, 10);
  strcat(res ,num);
  client.publish(inTopic,res);

closed as off-topic by Juraj, sempaiscuba, VE7JRO, Greenonline, per1234 Aug 30 '18 at 22:01

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "This question does not appear to be about Arduino, within the scope defined in the help center." – Juraj, sempaiscuba, VE7JRO, Greenonline, per1234
If this question can be reworded to fit the rules in the help center, please edit the question.

8

char *res = "up_cmd0_res:";

In principle, res should be a const char *. Const because it is pointing to a literal string, and you cannot (ar at least, you are not supposed to) change a literal string. The compiler should warn you on that... if you enable compiler warnings.

strcat(res ,num);

This is very wrong. strcat() expects as its first argument an pointer to a modifyable array large enough for the resulting concatenation (and the terminating \0). In other words, you have to allocate enough room for the whole string, then fill the beginning of the allocated array with the first substring, then only call strcat():

char res[24];
strcpy(res, "up_cmd0_res:");
strcat(res, num);

What happened is that you initially had, in memory, something like this:

<----- memory reserved for the string literal ---->
'u' 'p' '_' 'c' 'm' 'd' '0' '_' 'r' 'e' 's' ':' NUL
 ^-- res

On the first call to stracat(), you overwrite the terminating NUL and end up with

<----- memory reserved for the string literal ---->
'u' 'p' '_' 'c' 'm' 'd' '0' '_' 'r' 'e' 's' ':' '8' NUL
 ^-- res

Note that the new terminating NUL is now occupying a byte in memory that hasn't been allocated for this purpose. You have likely overwritten some global variable. Next time you call strcat(), you again replace the terminating NUL and continue to overwrite whatever happens to be stored in this part of the RAM. Expect a crash or program misbehavior.

  • thank you for your answer- can you please explain in what cases should I use char* instead of char ? and why def of res at the begining of the code as a predefined value- at the begining of function does not override is value ? – Guy . D Aug 19 '18 at 5:42
  • 1
    @Guy.D: 1. Re char* vs char[]: that's not a simple topic, but it's thoroughly covered in the section Arrays and Pointers of the C FAQ. Read it: it has in one place the answers to all the questions you are asking here. 2. Re def of res does not override is value: res is not a string, it's defined as an address and initialized at the beginning of the function to the place in RAM where the compiler stored the anonymous string literal. See also the other answers. – Edgar Bonet Aug 19 '18 at 8:11
6

Only the pointer, *res, is a local variable. The string "up_cmd0_res:" itself is elsewhere in RAM, stored as a literal and not meant to be modified.

Your strcat() call overwrites (extends) the literal each time you call it - which, by the way means that the growing string will be overwriting something else - whatever was following the string literal in global memory!!

You need to provide a character buffer within your function, large enough to hold "up_cmd0_res:" plus the longest thing you might ever append to it, plus one more byte for the zero-terminator. That buffer should be re-initialized on every function entry, before you append anything to it. Then make the strcat call to append to it. (These two calls will each manage the terminator for you).

The fact that this program didn't crash (or did it?) is only just by happenstance. If you call this function as it is initially written, enough times, your string literal will grow until it overwrites something important and either produces incorrect results (by over-writing some other data, or crashes (by overwriting the stack, especially the current function's return-address).

  • no. it did not crash – Guy . D Aug 18 '18 at 21:20
  • 3
    It will, in time. – Nick Gammon Aug 18 '18 at 22:35
4

It is incorrect to strcat into a field pointing to a literal. The code below is shorter, easier to understand, and does not corrupt memory:

 char res [20];
 sprintf (res, "up_cmd0_res:%i", counter2);
 client.publish(inTopic,res);

The field res needs to be long enough to hold the literal string (which is not modified doing it this way) plus the number you are adding to the end, and a terminating 0x00 byte.


can you please explain what "literal" is?

OK. A literal is when you literally put a string into a variable. For example:

char *res = "up_cmd0_res:";

So res isn't pointing to some variable, it is pointing to a literal string in the code. Now check this out:

void setup() {
  Serial.begin (115200);
  char * a = "foo";
  Serial.println (a);
  strcpy (a, "bar");
  Serial.println (a);
  char * b = "foo";
  Serial.println (b);
}

void loop()
{
}

Are you expecting this code to print:

foo
bar
foo

It should, right?, because the variable b contains "foo".

However it actually prints:

foo
bar
bar

This is because the strcpy has modified a literal string ("foo") to now contain "bar". The compiler is smart enough to think that every time it sees "foo" in the code it can place a single piece of memory with "foo" in it, because it is a literal (and literals don't change). But now you have modified that.

The compiler does give a warning:

/tmp/arduino_modified_sketch_905830/sketch_aug19a.ino:7:12: warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
 char * b = "foo";
            ^

Imagine if you were able to change the literal 5 so that every time you added 5 to something it actually added 42? That wouldn't be so great, right?


Your code was worse than that, because you didn't just replace a string with something the same length, you used strcat which replaces it with something longer.

  • thank you for your answer. can you please explain what "literal" is ? – Guy . D Aug 19 '18 at 5:41
2

Because you're changing a string literal. These literals are in static memory, so they persist between function calls.
On top of that, changing them results in undefined behaviour.

You're also writing out of the bounds of the string.

My recommendation:

void up_cmd0(){
  const size_t int_max_digits = floor(log10(pow(2, 8 * sizeof(int) - 1))) + 1;
  char num[int_max_digits + 2]; // max digits + minus sign + null terminator
  const char *str = "up_cmd0_res:";
  char buffer[strlen(str) + sizeof(num)] = {};
  strcat(buffer, str);
  itoa(counter2, num, 10);
  strcat(buffer, num);
  // ...
}

You shouldn't be using globals to get values from one function to the other (counter2), try to pass it as a parameter to the function.

Not the answer you're looking for? Browse other questions tagged or ask your own question.