fix: explicitly cast calloc_a variadic length arguments to size_t#30
Open
ptpt52 wants to merge 7 commits into
Open
fix: explicitly cast calloc_a variadic length arguments to size_t#30ptpt52 wants to merge 7 commits into
ptpt52 wants to merge 7 commits into
Conversation
ptpt52
force-pushed
the
master
branch
3 times, most recently
from
1e5afd8 to
2e60503
Compare
erikrk
reviewed
May 27, 2026
| service_add_srv(host, s, ttl); | ||
| if (s->txt && s->txt_len) | ||
| if (qtype == TYPE_ANY || qtype == TYPE_PTR) | ||
| service_add_ptr(service, service_instance_name(s), ttl); |
There was a problem hiding this comment.
Why was this changed back to service_instance_name(s) from host in my original PR? The service_instance_name function is not meant to be called repeatedly since it overwrites a static buffer and in principle corrupts both host and service. It happens to work anyway since it gets overwritten with identical data but it is not correct.
Comment on lines
+192
to
+193
| if (!append) | ||
| dns_packet_send(iface, to, 0, 0); |
There was a problem hiding this comment.
This looks a bit odd. I think if (count && !append) would be more clear.
| vlist_for_each_element(&interfaces, iface, node) { | ||
| s->t = 0; | ||
| service_reply_single(iface, NULL, s, announce_ttl, 1); | ||
| service_reply_single(iface, NULL, s, announce_ttl, 1, TYPE_ANY, false); |
There was a problem hiding this comment.
It looks like indentation got messed up here.
| return true; | ||
| } | ||
|
|
||
| bool dns_packet_answer(const char *name, int type, const uint8_t *rdata, uint16_t rdlength, int ttl) |
There was a problem hiding this comment.
This does not need to return bool since its return value is never used. Alternatively if we want to return a value, I think the return value from dns_packet_record should be propagated and we should do the same for dns_packet_additional.
| freeifaddrs(ifap); | ||
|
|
||
| dns_packet_send(iface, to, 0, 0); | ||
| if(!append) |
There was a problem hiding this comment.
I think this should be if (!append) to follow the code style.
On 64-bit architectures (e.g., aarch64), `size_t` is 8 bytes, while `int` remains 4 bytes. The `calloc_a` macro uses variadic arguments (`...`) to read memory allocation lengths via `va_arg(ap, size_t)`. If a 32-bit integer (like `tlen`, `dlen`, `txt_len`, or `0` in a ternary operator) is passed without an explicit cast, the compiler will not automatically promote it to 64-bit. As a result, `va_arg` reads 8 bytes, consuming the 32-bit value plus adjacent stack garbage. This triggers massive, uncontrolled memory allocations (e.g., attempting to malloc 508GB) and causes immediate OOM crashes. This patch explicitly casts all length arguments passed to `calloc_a` to `(size_t)` in `cache.c` and `service.c`, ensuring safe memory allocation across all architectures. Signed-off-by: Chen Minqiang <ptpt52@gmail.com>
`dns_packet_record_add` will return `NULL` if there is not enough room in the packet. This needs to be check to avoid crashes. Signed-off-by: Matthew Cather <mattbob4@gmail.com>
The priority and weight fields will be set to zero in accordance with behavior prior to introduction of the bug where calloc was used and in accordance with RFC 6763 Section 5 which prescribes priority and weight of zero for a single SRV record. Fixes: cef2502 ("fix excessive stack usage") Signed-off-by: Erik Karlsson <erik.karlsson@iopsys.eu>
Otherwise already sent questions will remain and packet keeps growing. Signed-off-by: Erik Karlsson <erik.karlsson@iopsys.eu>
* do not ignore legacy unicast queries, i.e., respond to them * comply with RFC 6762 Section 6.7 and repeat the query ID and the question in the response (RFC 6762 Section 6.7: ""If the source UDP port in a received Multicast DNS query is not port 5353, this indicates that the querier originating the query is a simple resolver... In this case, the Multicast DNS responder MUST send a UDP response directly back to the querier, via unicast, to the query packet's source IP address and port. This unicast response MUST be a conventional unicast response as would be generated by a conventional Unicast DNS server; for example, it MUST repeat the query ID and the question given in the query message.") * for legacy unicast queries, aggregate the answers in one response packet instead of sending multiple packets Signed-off-by: Mohd Husaam Mehdi <husaam.mehdi@iopsys.eu>
When responding to a PTR query, SRV and TXT records should be placed in the additional section and not the answer section. When responding to an A or AAAA query, addresses of mismatching type should be placed in the additional section and not the answer section. Respond to SRV and TXT queries. Only respond to PTR queries without instance specified (instance is not included in name of PTR records). PTR/SRV/TXT responses are always forced to yield predictable results. Rate limiting is only kept for the legacy ANY query handling. Fix a bug where the timeout was updated even if the response is forced and potential NULL pointer issues. Signed-off-by: Chen Minqiang <ptpt52@gmail.com> Signed-off-by: Erik Karlsson <erik.karlsson@iopsys.eu>
Replace the Variable Length Array (VLA) `orig_buffer[len]` with a fixed-size 1500-byte stack buffer (covering standard Ethernet MTU). This prevents potential stack exhaustion and kernel panics on memory-constrained devices (e.g., embedded routers) when receiving oversized UDP packets. For jumbo frames or anomalous packets (len > 1500), the code now gracefully falls back to heap allocation via `malloc()`. Additionally, introduced a `goto cleanup` pattern for error handling and early returns. This ensures that any dynamically allocated memory is safely and consistently freed across all function exit paths, eliminating the risk of memory leaks. Signed-off-by: Chen Minqiang <ptpt52@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On 64-bit architectures (e.g., aarch64),
size_tis 8 bytes, whileintremains 4 bytes. Thecalloc_amacro uses variadic arguments (...) to read memory allocation lengths viava_arg(ap, size_t).If a 32-bit integer (like
tlen,dlen,txt_len, or0in a ternary operator) is passed without an explicit cast, the compiler will not automatically promote it to 64-bit. As a result,va_argreads 8 bytes, consuming the 32-bit value plus adjacent stack garbage. This triggers massive, uncontrolled memory allocations (e.g., attempting to malloc 508GB) and causes immediate OOM crashes.This patch explicitly casts all length arguments passed to
calloc_ato(size_t)incache.candservice.c, ensuring safe memory allocation across all architectures.