squid
squid copied to clipboard
squid-cache
Fix type mismatch in new/delete of addrinfo::ai_addr
ERROR: AddressSanitizer: new-delete-type-mismatch...:
size of the allocated type: 28 bytes;
size of the deallocated type: 16 bytes.
in Ip::Address::InitAddr(addrinfo*&) src/ip/Address.cc:678
in Ip::Address::FreeAddr(addrinfo*&) src/ip/Address.cc:690
-
deleteused addrinfo::ai_addr type (i.e. sockaddr) - some
newcalls use sockaddr_in type - some
newcalls use sockaddr_in6 type
Mismatching new/delete types result in undefined behavior. However, since these are all PODs, the bug may not have resulted in runtime problems in most environments, even though sockaddr_in6 is usually larger than sockaddr and sockaddr_in tructure (e.g., 28 vs. 16 bytes).
I think there is merit to this patch, however it is probably incorrect:
If you see the code around line 593 (operator =):
// attempt to handle partially initialised addrinfo.
// such as those where data only comes from getsockopt()
if (s.ai_addr != nullptr) {
if (s.ai_addrlen == sizeof(struct sockaddr_in6)) {
operator=(*((struct sockaddr_in6*)s.ai_addr));
return true;
} else if (s.ai_addrlen == sizeof(struct sockaddr_in)) {
operator=(*((struct sockaddr_in*)s.ai_addr));
return true;
}
}
I think this patch should operate in a similar way and not blindly cast, but rely on the ai_addrlen to decide wheteher to cast. Could you look into this?
Sure, will update this.
Probably I missed something in my analysis,
I've based current change on assumption thats InitAddr and FreeAddr is pair function
and there is always expected that FreeAddr get ai->ai_addr as sockaddr_in6 type
Sure, will update this. Probably I missed something in my analysis, I've based current change on assumption thats
InitAddrandFreeAddris pair function and there is always expected thatFreeAddrgetai->ai_addrassockaddr_in6type
Maybe I missed out on that as well, but if that is the case, an assertion that the ai_addrlen be consistent with this assumption wouldn't hurt
Please also double check the adjusted PR title/description.
@jtstrs, have you seen this bug result in actual memory leaks with some common memory management library? I am just curious. It is a bug regardless of whether those memory leaks are common in some typical environments...
@rousskov
Actually there is just an new-delete-type-mismatch was caught with Address Sanitizer.
Memory leak was my assumption. Sorry, if it added some unclearance
There typical log from ASan for this case:
=================================================================
==(squid-1)==20157==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x603000115990 in thread T0:
object passed to delete has wrong type:
size of the allocated type: 28 bytes;
size of the deallocated type: 16 bytes.
#0 0x7f900f51b5a5 in operator delete(void*, unsigned long) (/usr/lib64/libasan.so.5+0x10a5a5)
1 0xefd1c7 in Ip::Address::FreeAddr(addrinfo*&) SQUID_PATH//squid-5.9/src/ip/Address.cc:690
0x603000115990 is located 0 bytes inside of 28-byte region [0x603000115990,0x6030001159ac)
allocated by thread T0 here:
#0 0x7f900f51a11f in operator new(unsigned long) (/usr/lib64/libasan.so.5+0x10911f)
1 0xefd05a in Ip::Address::InitAddr(addrinfo*&) SQUID_PATH/squid-5.9/src/ip/Address.cc:678
SUMMARY: AddressSanitizer: new-delete-type-mismatch (/usr/lib64/libasan.so.5+0x10a5a5) in operator delete(void*, unsigned long)
==(squid-1)==20157==HINT: if you don't care about these errors you may set ASAN_OPTIONS=new_delete_type_mismatch=0
Command: (squid-1) --kid squid-1 -f /etc/squid/squid_captive.conf
@rousskov Actually there is just an
new-delete-type-mismatchwas caught with Address Sanitizer. Memory leak was my assumption. Sorry, if it added some unclearance
It looks to me like you found a genuine issue. Not only that, Ip::Address is used quite a lot, which means it's also impactful. @rousskov 's comment is spot on, I reckon that the best path forward is to ensure ai_addrlen is consistent with the actual size of what is being stored in ai_addr, and to use that information. Long term, the whole class should probably be redesigned but that's against the actual short-term objective of fixing the problem quickly
Francesco: It looks to me like you found a genuine issue.
I would like to second Francesco's opinion: @jtstrs, your research/PR is very valuable, even if there are usually no memory leaks associated with the new/delete mismatch bugs you have uncovered.
@jtstrs, would you like to refactor PR changes to isolate ai_addr construction (including correct length setting and zeroing) and destruction into dedicated functions or should we take a stab at that first? Either way is fine; just let us know.
I'll prefer to stick an approach with encapsulation of construction/deleting ai_addr
I'll prefer to stick an approach with encapsulation of construction/deleting
ai_addr
OK, we will wait for your changes before polishing this up. No rush!
I'll check it patch with ASan again and add comment with results little bit later
Im little bit lost track of the conversation.
@rousskov @yadij, now I supposed to push my efforts to getting rid of a dynamic allocations for all usages of addrinfo related to recv and getsockname syscalls in scope of current PR?
now I supposed to push my efforts to getting rid of a dynamic allocations for all usages of
addrinforelated torecvandgetsocknamesyscalls in scope of current PR?
Personally, I recommend a slightly different approach: Find one addrinfo use case where addrinfo object is dynamically allocated, its length argument is supplied to some system call like recvfrom() to be updated, but the object as a whole is not used for anything (e.g. it is not then given to some other function to do something else). That is the simplest use case I can think of. Post a pull request dedicated to removing that one unnecessary addrinfo usage. Make sure the new code handles any reasonable (for the given context) address and errors. Once that PR lands, we will know how to address several similar (and possibly even all of the remaining) cases. If that plan works, eventually, we will get back to this PR with no dynamic addrinfo allocation cases left except for those this PR already handles correctly.
Icmp4::Recv() is a candidate but there might be even simpler use cases (Icmp4::Recv() does feed that addrinfo object to one of the fifty[^1] Ip::Address assignment operators, but it is possible that you will find another Ip::Address method that accomplishes what we want without adding one).
[^1]: "Fifty" is a wild exaggeration, of course. There are "only" eight AFAICT.
Sorry, didnt start yet. I've some medical issues. I'll continue work after ~2 weeks
I've started analysis of things which we discussed above, and I'm afraid, that I could'nt found any "dummy" usages of addrinfo structure, even Icmp* classes use information provided by recvfrom function for some logging purposes (I mean Icmp::Log(...)). So I'm not sure that there is possible to fully remove any usage of addrinfo, but I think that there is a few places, where we can get rid of from InitAddr usages.
Find one addrinfo use case where addrinfo object is dynamically allocated, its length argument is supplied to some system call like recvfrom() to be updated, but the object as a whole is not used for anything (e.g. it is not then given to some other function to do something else).
I've started analysis of things which we discussed above, and I'm afraid, that I could'nt found any "dummy" usages of addrinfo structure, even Icmp* classes use information provided by recvfrom function for some logging purposes (I mean Icmp::Log(...)). So I'm not sure that there is possible to fully remove any usage of addrinfo
Icmp::Log() does not take addrinfo. It takes Ip::Address. Thus, Icmp::Log() is a red herring on the path to removing excessive addrinfo use.
If you are studying Icmp4::Recv(), then decide what recvfrom() argument to use to store the "from" address and then how to convert that filled "from" address to Ip::Address (i.e. to preply.from). That new argument type should be byte-compatible with sockaddr structure layout (i.e. the type required by recvfrom()), of course. Most likely, that argument will not be allocated on the heap.
One possible (but not guaranteed) way to find an acceptable solution for the above problem is the following process:
- What is the overall best solution for correctly calling recvfrom() without using any 3rd party libraries?
- How to implement that solution in Icmp4::Recv() context?
Probably we could use sockaddr_storage type inside of Ip::Address class since its supposed to be wrapper for C api (according to @yadij said) and also wrap all system api calls inside of separate module like it's done in for example comm.cc:comm_udp_from, thus we get all interuction with this api in one place, also later there is possible to implement some wrappers for socket class instead of working with it handlers directly. What do you think?
Probably we could use
sockaddr_storagetype inside of Ip::Address class ... and also wrap all system api calls inside of separate module ... What do you think?
Ip::Address does need a lot of changes, but a major development like changing Ip::Address storage type is way outside this PR scope (regardless of its long-term merits). Correctly creating a new module to "wrap all system API calls" is also a huge undertaking. With all due respect, this kind of development should not be driven by a new Squid developer -- correct changes on those paths require too many Squid-specific (and difficult) decisions.
Please start small instead! Let's see how we can solve a few isolated/small problems first. Use Ip::Address as needed; add or adjust a method or two if you must, but stop if class changes snowball beyond that. Use sockaddr_storage as/if needed. Eliminate one or two specific addrinfo use cases. We will then decide how to generalize your fixes.
This PR changes may affect Ip::Address, its future rewrites, and even creation of new modules, of course, but I hope that we do not need to cross that bridge right here, right now, before we have settled a single addrinfo removal change.