Fix compiling errors while using gcc-7.2.0 for the crosstool-ng toolchain #1163
Conversation
|
|
|
That's very interesting @brainstorm , thanks. Although they look like "good" fixes for readability, the changes to cJSON & argtable are tweaks to upstream code. We have a policy of only modifying upstream code when we can't help it, because it makes it harder to apply updates to that code in the future. Rather than modifying these files, do you mind adding custom CFLAGS additions to the component.mk files to disable the offending warnings? You can see some examples of how to do this in (Also feel free to approaach the upstream maintainers about fixes, as well!) Very interested to know how you get on with gcc 7.2.0. |
components/mdns/mdns.c
Outdated
| @@ -569,7 +569,7 @@ static const uint8_t * _mdns_read_fqdn(const uint8_t * packet, const uint8_t * s | |||
| && (strcmp(buf, MDNS_DEFAULT_DOMAIN) != 0) | |||
| && (strcmp(buf, "ip6") != 0) | |||
| && (strcmp(buf, "in-addr") != 0)) { | |||
| snprintf((char*)name, MDNS_NAME_BUF_LEN, "%s.%s", name->host, buf); | |||
| snprintf((char*)name, MDNS_NAME_BUF_LEN+1, "%s.%s", name->host, buf); | |||
There was a problem hiding this comment.
I don't think this is correct, the destination is only MDNS_NAME_BUF_LEN size.
There was a problem hiding this comment.
I ran into a version of this one: https://bugzilla.redhat.com/show_bug.cgi?id=1431678
How would you express/fix it?
There was a problem hiding this comment.
Maybe using strncat instead?
There was a problem hiding this comment.
I'm actually having trouble reproducing this with my version of gcc 7.2 for some reason. But I don't have a lot of time to spend on this either.
The gcc changelog suggests that the other approach is to check the return value of snprintf to make sure the output wasn't truncated. This seems like the correct fix:
https://gcc.gnu.org/gcc-7/changes.html (search for -Wformat-truncated)
In case of failure, I think the correct behaviour should be to set name[0] = 0 and return NULL. @me-no-dev can you please confirm?
There was a problem hiding this comment.
That could work but it is essentially copying name->host on top of itself instead of appending
There was a problem hiding this comment.
ah right, I missed that this behaviour was appending in place. strlcat() may make the most sense then, with a check that the additional length will fit in the assigned buffer.
There was a problem hiding this comment.
@projectgus @negativekelvin Do you guys mean sth like this?:
- snprintf((char*)name, MDNS_NAME_BUF_LEN+1, "%s.%s", name->host, buf);
+ //snprintf((char*)name, MDNS_NAME_BUF_LEN, "%s.%s", name->host, buf);
+ strlcat(name->host, (char*)'.', sizeof(name->host));
+ strlcat(name->host, buf, sizeof(name->host));
Here's a good read from Theo: https://www.sudo.ws/todd/papers/strlcpy.html
I hope I applied it correctly for now :-S... is the check for truncation necessary in this case?
There was a problem hiding this comment.
Yes, like this looks good to me.
|
@brainstorm Are you still working on this? |
|
I'll be retaking this today among other things at some random café in Chapel Street, Melbourne ;) |
components/mdns/mdns.c
Outdated
| @@ -569,7 +569,9 @@ static const uint8_t * _mdns_read_fqdn(const uint8_t * packet, const uint8_t * s | |||
| && (strcmp(buf, MDNS_DEFAULT_DOMAIN) != 0) | |||
| && (strcmp(buf, "ip6") != 0) | |||
| && (strcmp(buf, "in-addr") != 0)) { | |||
| snprintf((char*)name, MDNS_NAME_BUF_LEN, "%s.%s", name->host, buf); | |||
| //snprintf((char*)name, MDNS_NAME_BUF_LEN, "%s.%s", name->host, buf); | |||
There was a problem hiding this comment.
Please delete this instead of commenting it (git has the history)
components/mdns/mdns.c
Outdated
| @@ -569,7 +569,9 @@ static const uint8_t * _mdns_read_fqdn(const uint8_t * packet, const uint8_t * s | |||
| && (strcmp(buf, MDNS_DEFAULT_DOMAIN) != 0) | |||
| && (strcmp(buf, "ip6") != 0) | |||
| && (strcmp(buf, "in-addr") != 0)) { | |||
| snprintf((char*)name, MDNS_NAME_BUF_LEN, "%s.%s", name->host, buf); | |||
| //snprintf((char*)name, MDNS_NAME_BUF_LEN, "%s.%s", name->host, buf); | |||
| strlcat(name->host, (char*)'.', sizeof(name->host)); | |||
There was a problem hiding this comment.
I think you mean "." instead of (char *)'.'. The latter is a pointer to the ASCII value of dot, which is certainly not valid memory. :)
There was a problem hiding this comment.
Certainly, fixed ;)
|
I would also like to verify this PR + a toolchain built using gcc 7, and including these changes. However, I am not sure what patch is required to generate the |
|
@paulreimer I guess you are looking for this one? Not sure if it will generate the .a accordingly though, haven't delved yet into psram with the esp32 at all: Try to apply it against gcc 7.x and let me know (perhaps via issuing a PR to this same branch)? |
|
Just to confirm what @brainstorm said, if the toolchain has the patch linked above and you compile with the Angus |
|
Thanks for the updates, @brainstorm . I've pushed this into our internal review & merge queue. |
|
Excellent! Please let me know when you push the accompanying crosstool-ng PR... (hopefully upstream?) so I can put together the Dockcross container for xtensa: |
|
No problems. As I explained a bit in the linked PR, we probably won't be doing anything to merge gcc 7 toolchain support until upstream decides to act on this themselves. But you're of course welcome to keep working on this (or talk to the upstreams) independently. |
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges #1163
* Fixes compilation errors/warnings with gcc 7.2.0 Merges #1163
|
Squashed and cherry-picked in the above two commits. Thanks @brainstorm ! |
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges #1163
* Fixes compilation errors/warnings with gcc 7.2.0 Merges #1163
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
…chain * Change snprintf for strlcat does not complain w/gcc7.2.0 and it is safer, thanks @projectgus * Use proper quotes for character literals Merges espressif/esp-idf#1163 * Original commit: espressif/esp-idf@519edc3
Shouldn't break anything on current crosstool-ng gcc<5.2.x toolchains... I guess.
This is the result of following and fixing the stream of errors that this user observed:
https://www.esp32.com/viewtopic.php?f=2&t=3362
Apparently gcc7.2.0 flags as errors that i.e
rcsidis declared but not used... I guess I'm missing some more lax gcc flag in crosstool-ng, I'll followup with the accompanying PR there too (crosstool-ng toolchain).Meanwhile, I'm leaving a working
.configfor crosstool-ng here (just bumping versions and applying this GCC patch, if someone is interested):