-2

This question may not make sense without the entire code but I am going to try my best to explain it. As a bit of background the code is a snippet from the OpenThread project on GitHub but I believe that standard c/c++ principles still apply here.

The purpose of the code is to append a piece of data (in this case a string) onto an OpenThread (ot) message. As arguments it takes an otMessage, a buffer (with the data) and the length of the buffer to be copied into the otMessage. I am not really sure how otMessageAppend works and it is completely possible that the error is in the way that it reads the buffer, if this is the case nothing can be done about it.

The following c++ code was the starting point:

char command[] = "abcdef";
SuccessOrExit(error = otMessageAppend(message, &command, (uint16_t)strlen(command)*sizeof(char)));

When receiving the message at the other end I get abcdef

When passed abcdef as argv[4] this code works perfectly: 

char command[strlen(argv[4])+1];//+1 for the null terminator
strcpy(command, argv[4]);
SuccessOrExit(error = otMessageAppend(message, &command, (uint16_t)strlen(command)*sizeof(char)));

However trying to allocate the memory using malloc causes garbage to come out the other end (the correct number of bytes but not the correct data):

char *command;
command = (char *) malloc(strlen(argv[4])+1);
strcpy(command, argv[4]);
SuccessOrExit(error = otMessageAppend(message, &command, (uint16_t)strlen(command)*sizeof(char)));

I have a few questions about this:

  1. Is there anything wrong with declaring memory using char array[size] from what I understand the difference is that using [] will allocate memory in the stack whereas malloc allocates the memory in the heap.
  2. If I should be using malloc how can I ensure that the correct piece of memory will get "appended to the message" and not just garbage

If its vital to know how otAppendMessage works I can dig through the source code and find it.

Campbell Wray
  • 75
  • 1
  • 9
  • `&command` has a different meaning when `command` is a pointer. You get a pointer to the pointer, not a pointer to the string. – Bo Persson Apr 18 '17 at 08:55
  • 3
    Are you programming C or C++? They are different languages and truly behave differently. Considering that you use [variable-length arrays](https://en.wikipedia.org/wiki/Variable-length_array) and `malloc` I assume you actually program in C (in which case you should read [this discussion about casting the result of `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc))? – Some programmer dude Apr 18 '17 at 08:56
  • If it's c++ then why not `new` instead of `malloc`? And why not `std::string` instead of `char*`? – Rogus Apr 18 '17 at 08:56
  • Compile with warnings enabled! And understand the warnings. – Antti Haapala -- Слава Україні Apr 18 '17 at 08:59
  • Either way, without knowing the exact declaration of `otMessageAppend` (a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve) would be helpful) you might want to enable more warning when building. Using the address-of operator for true arrays (not pointers) is usually not something one would do, and if the arguments doesn't match you would get a warning about it (or an error in C++). – Some programmer dude Apr 18 '17 at 08:59
  • @AnttiHaapala: If the receiving parameter has type `void*`, I don't think there's anything to warn about in that code. – Kerrek SB Apr 18 '17 at 09:00
  • @KerrekSB true that - but it could perhaps be changed to something like `char *` or `uint8_t *` if these are binary buffers. – Antti Haapala -- Слава Україні Apr 18 '17 at 09:01
  • Its C++, I only have about 2 days worth of experience with C++ and not much more with C. Because it is such a small part of the repository, it is difficult to know if I am doing things correctly – Campbell Wray Apr 18 '17 at 09:02
  • if this is POSIX specific code, you could use `char *command = strdup(argv[4]);` - but then, why do you need to *copy* the string at all, just use 'argv[4]` there? – Antti Haapala -- Слава Україні Apr 18 '17 at 09:06
  • If you use C++, then ditch that stuff and take a look at std::vector, std::array, std::list and std::string. Take on arrays later, and only if you really need to, and then only when wrapped in a basic class (that is comparable to std::vector). Also, learn the differences between C and C++. Things that are good in C can be bad in C++, even when they work. – Aziuth Apr 18 '17 at 09:07
  • @AnttiHaapala, no it is not POSIX specific, probably should have specified that – Campbell Wray Apr 18 '17 at 09:07
  • @AnttiHaapala sorry I missed the second bit of your comment, I was copying the string to test if that would work, completely forgot to remove it. – Campbell Wray Apr 18 '17 at 09:11
  • @Aziuth thanks for the advice, I will look through those methods and see where it gets me – Campbell Wray Apr 18 '17 at 09:11

1 Answers1

3

Replace &command by command.

Both snippets are bad, but the first one happens to work because of the address of the array is the same as the address of its first element, and if the parameter has type void*, both expressions result in the same converted argument. In the second snippet, the address of the pointer is most certainly not equal to the value of the pointer (because of malloc's no-alias guarantee).

Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
  • If the snippets are bad is there something I could be doing to improve them, Is there any advantage of the `malloc` way of doing it – Campbell Wray Apr 18 '17 at 08:58
  • 1
    @CampbellWray: What you could do to improve them is to replace `&command` by `command`, as the answer says. Whether to use automatic or dynamic memory depends on your larger design requirements (string size, lifetime); you need to make that decision yourself. – Kerrek SB Apr 18 '17 at 08:59
  • 1
    @CampbellWray you need to remove `&` in **both** versions. – Antti Haapala -- Слава Україні Apr 18 '17 at 09:00
  • Thanks @AnttiHaapala and Kerrek, I am building on top of someone else's code so I am largely following what they had, hence the `&`. – Campbell Wray Apr 18 '17 at 09:03
  • "Hence the `&`". Perhaps you shouldn't take advice from that code. – Antti Haapala -- Слава Україні Apr 18 '17 at 09:05
  • 2
    @CampbellWray: I shoud say that it's probably more effective to program from insight and with deliberation rather than by pattern matching, syntax guessing and following recipes. The subject is complex, and it is unlikely that you'll get very far by piecing together opaque building blocks. – Kerrek SB Apr 18 '17 at 09:06
  • @AnttiHaapala, the original code didn't work properly, that's why I am trying to get it working with almost 0 experience – Campbell Wray Apr 18 '17 at 09:06
  • Thanks for the help guys, after removing the & it worked perfectly, I just went a bit overboard copying memory all over the place to debug my issues – Campbell Wray Apr 18 '17 at 09:17