Fix type mismatch in new/delete of addrinfo::ai_addr

[kenballus]

new/delete type mismatches are UB. Fix an instance of this
problem that occurs when sockaddr_in6 is allocated, but
sockaddr is deallocated, by always allocating/deallocating
sockaddr_storage.

AddressSanitizer: new-delete-type-mismatch:
  object passed to delete has wrong type:
  size of the allocated type:   28 bytes;
  size of the deallocated type: 16 bytes.
#0 0xaaaad1a8db54 in operator delete(void*, unsigned long)
#1 0xaaaad287a668 in Ip::Address::FreeAddr(addrinfo*&)
    src/ip/Address.cc:710:22

[kenballus]

The relevant UBSan error:

==21==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x503000004990 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 0xaaaad1a8db54 in operator delete(void*, unsigned long) (/usr/local/squid/sbin/squid+0x118db54) (BuildId: 94d646c4f5dcfdbf0ae64bf5b5d47dbd894a01a6)
    #1 0xaaaad287a668 in Ip::Address::FreeAddr(addrinfo*&) /app/squid/src/ip/Address.cc:710:22
    #2 0xaaaad269b3e4 in comm_openex(int, int, Ip::Address&, int, char const*) /app/squid/src/comm.cc:438:5
    #3 0xaaaad269bc64 in comm_open_listener(int, int, Ip::Address&, int, char const*) /app/squid/src/comm.cc:284:12
    #4 0xaaaad1adcb7c in Dns::Init() /app/squid/src/dns_internal.cc:1566:26
    #5 0xaaaad212f0c4 in mainInitialize() /app/squid/src/main.cc:1113:5
    #6 0xaaaad2128ff8 in SquidMain(int, char**) /app/squid/src/main.cc:1624:5
    #7 0xaaaad21275ec in SquidMainSafe(int, char**) /app/squid/src/main.cc:1303:16
    #8 0xaaaad21275b4 in main /app/squid/src/main.cc:1291:12
    #9 0xffffac322298  (/lib/aarch64-linux-gnu/libc.so.6+0x22298) (BuildId: cd750d7a4e9c892b0cb64122dbb779d4cc2bbcbe)
    #10 0xffffac322378 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x22378) (BuildId: cd750d7a4e9c892b0cb64122dbb779d4cc2bbcbe)
    #11 0xaaaad19aac6c in _start (/usr/local/squid/sbin/squid+0x10aac6c) (BuildId: 94d646c4f5dcfdbf0ae64bf5b5d47dbd894a01a6)

0x503000004990 is located 0 bytes inside of 28-byte region [0x503000004990,0x5030000049ac)
allocated by thread T0 here:
    #0 0xaaaad1a8cea4 in operator new(unsigned long) (/usr/local/squid/sbin/squid+0x118cea4) (BuildId: 94d646c4f5dcfdbf0ae64bf5b5d47dbd894a01a6)
    #1 0xaaaad2878f3c in Ip::Address::getAddrInfo(addrinfo*&, int) const /app/squid/src/ip/Address.cc:647:42
    #2 0xaaaad26991a4 in comm_openex(int, int, Ip::Address&, int, char const*) /app/squid/src/comm.cc:377:10
    #3 0xaaaad269bc64 in comm_open_listener(int, int, Ip::Address&, int, char const*) /app/squid/src/comm.cc:284:12
    #4 0xaaaad1adcb7c in Dns::Init() /app/squid/src/dns_internal.cc:1566:26
    #5 0xaaaad212f0c4 in mainInitialize() /app/squid/src/main.cc:1113:5
    #6 0xaaaad2128ff8 in SquidMain(int, char**) /app/squid/src/main.cc:1624:5
    #7 0xaaaad21275ec in SquidMainSafe(int, char**) /app/squid/src/main.cc:1303:16
    #8 0xaaaad21275b4 in main /app/squid/src/main.cc:1291:12
    #9 0xffffac322298  (/lib/aarch64-linux-gnu/libc.so.6+0x22298) (BuildId: cd750d7a4e9c892b0cb64122dbb779d4cc2bbcbe)
    #10 0xffffac322378 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x22378) (BuildId: cd750d7a4e9c892b0cb64122dbb779d4cc2bbcbe)
    #11 0xaaaad19aac6c in _start (/usr/local/squid/sbin/squid+0x10aac6c) (BuildId: 94d646c4f5dcfdbf0ae64bf5b5d47dbd894a01a6)

SUMMARY: AddressSanitizer: new-delete-type-mismatch (/usr/local/squid/sbin/squid+0x118db54) (BuildId: 94d646c4f5dcfdbf0ae64bf5b5d47dbd894a01a6) in operator delete(void*, unsigned long)

[rousskov]

Thank you for attacking this bug! PR #1671 tried to fix this problem as well, but used ai_addrlen unstead of ai_family to figure out the right type. It was later decided that using ai_addrlen is not the right answer because its value is unreliable. I have not checked whether ai_family may suffer from similar problems.

• If ai_family suffers from similar problems, then PR 1671 proposed long-term solution (i.e. get rid of these dynamic allocations) is probably still the best way forward.
• Otherwise, PR 1671-like changes that isolate ai_addr allocations/deallocations and warn us about unsupported ai_family values are warranted in this PR. See AllocateAddrMember() and FreeAddrMember() functions there.

@kenballus, do you think ai_family is always correct in this context? If yes, please incorporate AllocateAddrMember() and FreeAddrMember() changes from #1671 while using ai_family instead of ai_addrlen. Doing so will, among other positive effects, fix one delete case that this PR have missed. Please also borrow PR 1671 title/description, with any necessary adjustments.

[kenballus]

I think the easiest solution here is just to new and delete as struct sockaddr_storage. This means that new/delete always match, but the rest of the code doesn't need to change.

Just force-pushed.

[kenballus]

Unfortunately, I don't think ai_family is reliable. For some reason, the patch didn't behave as expected when I checked ai_family against AF_INET6. Best path forward is probably just to ensure that allocation/deallocation doesn't need to know anything about the type of the struct other than its size.

[kenballus]

@rousskov do you think this is a suitable solution to the problem? I'd prefer not to make sweeping changes here if they can be avoided.

[rousskov]

@rousskov do you think this is a suitable solution to the problem?

I need more time to answer your question.

I'd prefer not to make sweeping changes here if they can be avoided.

I believe you and I share that preference, although our definitions of "sweeping changes" may differ :-). At any rate, the currently proposed sockaddr_storage-based solution deserves a consideration, at least as a starting point. Please give me a few workdays to come back and review it properly.

[kenballus]

I believe you and I share that preference, although our definitions of "sweeping changes" may differ :-)

Basically anything more than a few lines is "sweeping" to me :)

At any rate, the currently proposed sockaddr_storage-based solution deserves a consideration, at least as a starting point.

This is the solution that the Linux kernel took to a similar problem. See https://lwn.net/Articles/997094/

Please give me a few workdays to come back and review it properly.

Of course.

Thanks!
Ben

[yadij]

@rousskov , LGTM over to you now.

[rousskov]

I cannot think of a better surgical solution to this problem. Thank you.

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7

[squidadm]

queued for backport to v7