Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address Compiler Errors under GCC and clang/LLVM for Maintainer Mode Builds #11

Open
gerickson opened this issue Nov 1, 2023 · 1 comment
Labels
build Build-related issue help wanted Extra attention is needed

Comments

@gerickson
Copy link
Contributor

gerickson commented Nov 1, 2023

Under maintainer mode builds (--enable-maintainer-mode), which is the default when using bootstrap-configure, connman has the following clang/LLVM and GCC errors (since maintainer mode enables -Werror):

clang/LLVM

src/iptables.csrc/iptables.c:805:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict >= entry_before->offset)
                            ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~
src/iptables.c:808:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict > entry_before->offset)
                            ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~
src/iptables.c:805:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict >= entry_before->offset)
                            ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~
src/iptables.c:808:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict > entry_before->offset)
                            ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~
src/iptables.c:805:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict >= entry_before->offset)
                            ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~
src/iptables.c:808:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict > entry_before->offset)
                            ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~
src/dnsproxy.c:439:10: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
        if (len < DNS_QUESTION_SIZE + 1)
            ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~
src/dnsproxy.c:462:11: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
                if (len < sizeof(*rr))
                    ~~~ ^ ~~~~~~~~~~~
src/dnsproxy.c:523:15: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
        else if (err != len || dns_len != (len - offset))
                 ~~~ ^  ~~~
src/dnsproxy.c:1066:19: error: comparison of integers of different signs: 'long' and 'unsigned long' [-Werror,-Wsign-compare]
        if ((eptr - ptr) < DNS_QUESTION_SIZE)
             ~~~~~~~~~~  ^ ~~~~~~~~~~~~~~~~~
src/iptables.c:805:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict >= entry_before->offset)
                            ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~
src/iptables.c:808:19: error: comparison of integers of different signs: 'int' and 'unsigned int' [-Werror,-Wsign-compare]
                        if (t->verdict > entry_before->offset)
                            ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~
src/dnsproxy.c:2524:26: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
                        } else if (bytes_recv < sizeof(reply_len))
                                   ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~

GCC

src/iptables.c: In function ‘update_targets_reference’:
src/iptables.c:805:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  805 |                         if (t->verdict >= entry_before->offset)
      |                                        ^~
src/iptables.c:808:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  808 |                         if (t->verdict > entry_before->offset)
      |                                        ^
  CC       src/tools_dnsproxy_standalone-detect.o
  CC       src/tools_dnsproxy_standalone-inet.o
src/iptables.c: In function ‘update_targets_reference’:
src/iptables.c:805:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  805 |                         if (t->verdict >= entry_before->offset)
      |                                        ^~
  CC       src/tools_dnsproxy_standalone-dhcp.o
unit/test-iptables.c:72:1: error: ‘static’ is not at beginning of declaration [-Werror=old-style-declaration]
   72 | int static xt_match_parse(int c, char **argv, int invert, unsigned int *flags,
      | ^~~
unit/test-iptables.c:78:1: error: ‘static’ is not at beginning of declaration [-Werror=old-style-declaration]
   78 | int static xt_target_parse(int c, char **argv, int invert, unsigned int *flags,
      | ^~~
  CC       src/tools_dnsproxy_standalone-dhcpv6.o
unit/test-iptables.c: In function ‘xtables_option_mfcall’:
unit/test-iptables.c:206:50: error: parameter ‘m’ set but not used [-Werror=unused-but-set-parameter]
  206 | void xtables_option_mfcall(struct xtables_match *m)
      |                            ~~~~~~~~~~~~~~~~~~~~~~^
unit/test-iptables.c: In function ‘xtables_option_tfcall’:
unit/test-iptables.c:218:51: error: parameter ‘t’ set but not used [-Werror=unused-but-set-parameter]
  218 | void xtables_option_tfcall(struct xtables_target *t)
      |                            ~~~~~~~~~~~~~~~~~~~~~~~^
unit/test-iptables.c: In function ‘xtables_option_mpcall’:
unit/test-iptables.c:231:47: error: parameter ‘m’ set but not used [-Werror=unused-but-set-parameter]
  231 |                         struct xtables_match *m, void *fw)
      |                         ~~~~~~~~~~~~~~~~~~~~~~^
unit/test-iptables.c: In function ‘xtables_option_tpcall’:
unit/test-iptables.c:244:48: error: parameter ‘t’ set but not used [-Werror=unused-but-set-parameter]
  244 |                         struct xtables_target *t, void *fw)
      |                         ~~~~~~~~~~~~~~~~~~~~~~~^
  CC       src/tools_dnsproxy_standalone-rtnl.o
src/iptables.c:808:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  808 |                         if (t->verdict > entry_before->offset)
      |                                        ^
unit/test-iptables.c: In function ‘getsockopt’:
unit/test-iptables.c:310:58: error: parameter ‘optval’ set but not used [-Werror=unused-but-set-parameter]
  310 | int getsockopt(int sockfd, int level, int optname, void *optval,
      |                                                    ~~~~~~^~~~~~
  CC       src/tools_dnsproxy_standalone-proxy.o
  CC       src/tools_dnsproxy_standalone-utsname.o
src/iptables.c: In function ‘update_targets_reference’:
src/iptables.c:805:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  805 |                         if (t->verdict >= entry_before->offset)
      |                                        ^~
src/iptables.c:808:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  808 |                         if (t->verdict > entry_before->offset)
      |                                        ^
src/iptables.c: In function ‘iptables_append_rule’:
src/iptables.c:1345:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1345 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1347:9: note: here
 1347 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1348:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1348 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1350:9: note: here
 1350 |         default:
      |         ^~~~~~~
src/iptables.c: In function ‘prepare_rule_inclusion’:
src/iptables.c:1288:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1288 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1290:9: note: here
 1290 |         case AF_INET6:
      |         ^~~~
src/iptables.c: In function ‘iptables_insert_rule’:
src/iptables.c:1399:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1399 |                 if (new_entry->entry)
      |                    ^
  CC       src/tools_dnsproxy_standalone-6to4.o
src/iptables.c:1401:9: note: here
 1401 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1402:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1402 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1404:9: note: here
 1404 |         default:
      |         ^~~~~~~
src/iptables.c: In function ‘iptables_append_rule’:
src/iptables.c:1345:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1345 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1347:9: note: here
 1347 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1348:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1348 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1350:9: note: here
 1350 |         default:
      |         ^~~~~~~
src/iptables.c: In function ‘prepare_rule_inclusion’:
src/iptables.c:1288:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1288 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1290:9: note: here
 1290 |         case AF_INET6:
      |         ^~~~
src/iptables.c: In function ‘iptables_insert_rule’:
src/iptables.c:1399:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1399 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1401:9: note: here
 1401 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1402:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1402 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1404:9: note: here
 1404 |         default:
      |         ^~~~~~~
  CC       src/tools_dnsproxy_standalone-ippool.o
  CC       src/tools_dnsproxy_standalone-bridge.o
  CC       src/tools_dnsproxy_standalone-nat.o
src/iptables.c: In function ‘iptables_append_rule’:
src/iptables.c:1345:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1345 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1347:9: note: here
 1347 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1348:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1348 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1350:9: note: here
 1350 |         default:
      |         ^~~~~~~
src/iptables.c: In function ‘prepare_rule_inclusion’:
src/iptables.c:1288:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1288 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1290:9: note: here
 1290 |         case AF_INET6:
      |         ^~~~
src/iptables.c: In function ‘iptables_insert_rule’:
src/iptables.c:1399:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1399 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1401:9: note: here
 1401 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1402:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1402 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1404:9: note: here
 1404 |         default:
      |         ^~~~~~~
src/dnsproxy.c: In function ‘update_cached_ttl’:
src/iptables.c: In function ‘update_targets_reference’:
src/dnsproxy.c:439:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
  439 |         if (len < DNS_QUESTION_SIZE + 1)
      |                 ^
src/dnsproxy.c:462:25: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
  462 |                 if (len < sizeof(*rr))
      |                         ^
src/dnsproxy.c: In function ‘send_cached_response’:
src/dnsproxy.c:523:22: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
  523 |         else if (err != len || dns_len != (len - offset))
      |                      ^~
src/dnsproxy.c: In function ‘append_data’:
src/dnsproxy.c:659:29: error: operand of ‘?:’ changes signedness from ‘long int’ to ‘size_t’ {aka ‘long unsigned int’} due to unsignedness of other operand [-Werror=sign-compare]
  659 |                 len = dot ? dot - data : strlen(data);
      |                             ^~~~~~~~~~
src/dnsproxy.c: In function ‘parse_response’:
src/iptables.c:805:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  805 |                         if (t->verdict >= entry_before->offset)
      |                                        ^~
src/iptables.c:808:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  808 |                         if (t->verdict > entry_before->offset)
      |                                        ^
src/dnsproxy.c:1066:26: error: comparison of integer expressions of different signedness: ‘long int’ and ‘long unsigned int’ [-Werror=sign-compare]
 1066 |         if ((eptr - ptr) < DNS_QUESTION_SIZE)
      |                          ^
src/dnsproxy.c: In function ‘dns_reply_fixup_domains’:
src/dnsproxy.c:2034:23: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
 2034 |         if (reply_len < header_len + 1)
      |                       ^
src/iptables.c: In function ‘final_check_xt_modules’:
src/iptables.c:3190:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3190 |         struct xtables_rule_match *rm;
      |                                    ^~
src/dnsproxy.c: In function ‘tcp_server_event’:
src/dnsproxy.c:2524:47: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
 2524 |                         } else if (bytes_recv < sizeof(reply_len))
      |                                               ^
src/iptables.c: In function ‘final_check_xt_modules’:
src/iptables.c:3190:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3190 |         struct xtables_rule_match *rm;
      |                                    ^~
src/iptables.c: In function ‘final_check_xt_modules’:
src/iptables.c:3190:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3190 |         struct xtables_rule_match *rm;
      |                                    ^~
src/iptables.c: In function ‘parse_xt_modules’:
src/iptables.c:3090:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3090 |         struct xtables_rule_match *rm;
      |                                    ^~
src/iptables.c: In function ‘iptables_append_rule’:
src/iptables.c: In function ‘parse_xt_modules’:
src/iptables.c:1345:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1345 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1347:9: note: here
 1347 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1348:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1348 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1350:9: note: here
 1350 |         default:
      |         ^~~~~~~
src/iptables.c: In function ‘prepare_rule_inclusion’:
src/iptables.c:1288:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1288 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1290:9: note: here
 1290 |         case AF_INET6:
      |         ^~~~
src/iptables.c: In function ‘iptables_insert_rule’:
src/iptables.c:1399:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1399 |                 if (new_entry->entry)
      |                    ^
src/iptables.c:1401:9: note: here
 1401 |         case AF_INET6:
      |         ^~~~
src/iptables.c:1402:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1402 |                 if (new_entry->entry6)
      |                    ^
src/iptables.c:1404:9: note: here
 1404 |         default:
      |         ^~~~~~~
src/iptables.c:3090:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3090 |         struct xtables_rule_match *rm;
      |                                    ^~
src/iptables.c: In function ‘parse_xt_modules’:
src/iptables.c:3090:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3090 |         struct xtables_rule_match *rm;
      |                                    ^~
src/iptables.c: In function ‘prepare_target’:
src/iptables.c:2686:14: error: variable ‘is_builtin’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |              ^~~~~~~~~~
src/iptables.c:2686:26: error: variable ‘is_user_defined’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |                          ^~~~~~~~~~~~~~~
src/iptables.c:2687:16: error: variable ‘chain_head’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2687 |         GList *chain_head = NULL;
      |                ^~~~~~~~~~
src/iptables.c: In function ‘prepare_target’:
src/iptables.c:2686:14: error: variable ‘is_builtin’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |              ^~~~~~~~~~
src/iptables.c:2686:26: error: variable ‘is_user_defined’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |                          ^~~~~~~~~~~~~~~
src/iptables.c:2687:16: error: variable ‘chain_head’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2687 |         GList *chain_head = NULL;
      |                ^~~~~~~~~~
src/iptables.c: In function ‘prepare_target’:
src/iptables.c:2686:14: error: variable ‘is_builtin’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |              ^~~~~~~~~~
src/iptables.c:2686:26: error: variable ‘is_user_defined’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |                          ^~~~~~~~~~~~~~~
src/iptables.c:2687:16: error: variable ‘chain_head’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2687 |         GList *chain_head = NULL;
      |                ^~~~~~~~~~
src/iptables.c: In function ‘final_check_xt_modules’:
src/iptables.c:3190:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3190 |         struct xtables_rule_match *rm;
      |                                    ^~
src/iptables.c: In function ‘parse_xt_modules’:
src/iptables.c:3090:36: error: variable ‘rm’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 3090 |         struct xtables_rule_match *rm;
      |                                    ^~
src/iptables.c: In function ‘prepare_target’:
src/iptables.c:2686:14: error: variable ‘is_builtin’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |              ^~~~~~~~~~
src/iptables.c:2686:26: error: variable ‘is_user_defined’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2686 |         bool is_builtin, is_user_defined;
      |                          ^~~~~~~~~~~~~~~
src/iptables.c:2687:16: error: variable ‘chain_head’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
 2687 |         GList *chain_head = NULL;
      |                ^~~~~~~~~~

Fix these such that continuous integration (CI) succeeds.

@gerickson
Copy link
Contributor Author

@LaakkonenJussi, any thoughts on addressing the -Werror=clobbered errors?

@gerickson gerickson added the help wanted Extra attention is needed label Nov 1, 2023
gerickson added a commit that referenced this issue Nov 2, 2023
This qualifies the call to '__connman_timeserver_sync' with an
'is_connected' check on the new state of the service in
'__connman_service_ipconfig_indicate_state'. In fact, this leverages
the same conditional that 'nameserver_add_all' is also subject to.
This avoids a heap-use-after-free segmentation fault that can occur
when a network service goes away following link loss or a disable of
the corresponding technology.

Without this qualification, the network service going away will invoke
'__connman_timeserver_sync' with the
'CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE' reason as it cycles from
online/ready to disconnect. As currently implemented, the timeserver
code interprets this as a request to initiate time-of-day synchroninzation
with time services using the name and time services of that disappearing
service. As a result, this has two downstream impacts:

  1. In a multi-service connectivity environment, the junior service that
     was promoted when the senior service was disconnected and was set as
     the active service for time services when 'default_changed' was
     invoked is undesireably displaced by the disconnecting service.

  2. The implication of (1) is that the time service now holds a weak
     reference to the disappearing service in 'ts_service' rather than
     the promoted and new default service. Consequentl, when 'sync_next'
     is next called by time services, it will attempt to get the name
     or time servers for the disconnecting service. Unfortunately,
     these which have gone away, as 'service_free' was invoked on the
     disconnected service after it was wound down. It is the access to
     these name or time servers on the deallocated service object that
     causes the heap-use-after-free segmentation fault.

The address sanitizer report is as follows:

    =================================================================
    ==526==ERROR: AddressSanitizer: heap-use-after-free on address 0x72603898 at pc 0x0009b451 bp 0x7e9678f8 sp 0x7e9678fc
    READ of size 4 at 0x72603898 thread T0
        #0 0x9b44e in connman_service_get_nameservers:service.c:2896 (/usr/sbin/connmand+0x9b44e)
        #1 0xd485e in ts_set_nameservers:timeserver.c:387 (/usr/sbin/connmand+0xd485e)
        #2 0xd4930 in sync_next:timeserver.c:187 (/usr/sbin/connmand+0xd4930)
        #3 0xd4e6a in resolv_result:timeserver.c:141 (/usr/sbin/connmand+0xd4e6a)
        #4 0x37b5a in sort_and_return_results:gresolv.c:522 (/usr/sbin/connmand+0x37b5a)
        #5 0x385e4 in query_timeout:gresolv.c:547 (/usr/sbin/connmand+0x385e4)

    0x72603898 is located 88 bytes inside of 408-byte region [0x72603840,0x726039d8)
    freed by thread T0 here:
       #0 0x76a0d264  (/lib/libasan.so.8+0x8d264)

    previously allocated by thread T0 here:
        #0 0x76a0da6c in calloc (/lib/libasan.so.8+0x8da6c)
        #1 0x925b4 in connman_service_create (/usr/sbin/connmand+0x925b4)
        #2 0x9294c  (/usr/sbin/connmand+0x9294c)
        #3 0x9e844  (/usr/sbin/connmand+0x9e844)
        #4 0x7dda6  (/usr/sbin/connmand+0x7dda6)
        #5 0x7ea18 in connman_network_set_group (/usr/sbin/connmand+0x7ea18)
        #6 0x3da36  (/usr/sbin/connmand+0x3da36)
        #7 0x3db56  (/usr/sbin/connmand+0x3db56)
        #8 0xd057c  (/usr/sbin/connmand+0xd057c)
        #9 0xd0882  (/usr/sbin/connmand+0xd0882)
        #10 0xd28da  (/usr/sbin/connmand+0xd28da)
        #11 0xd2ca4  (/usr/sbin/connmand+0xd2ca4)

   SUMMARY: AddressSanitizer: heap-use-after-free (/usr/sbin/connmand+0x9b44e) in connman_service_get_nameservers
gerickson added a commit that referenced this issue Nov 2, 2023
This changes the semantics of dealing with 'ts_service', the active
network service against which time-of-day synchronization is run,
from a weak (that is, simple assignment) reference to a strong (that
is, uses 'connman_service_{ref,unref}') reference.

This avoids a stale reference that can occur in failure paths
should the time service find itself 'ts_service' referencing
a disconnecting and, consequently, disappearing service.

This stale reference would otherwise lead to a heap-use-after-free
fault when the time services attempts to access the name or time
services associated with the stale service.

By promoting the weak reference to a strong reference, a potential
service deallocation is deferred until 'timeserver_stop' is invoked
and the reference to the service released.

The address sanitizer report associated with the potential heap-use-
after-free is as follows:

    =================================================================
    ==526==ERROR: AddressSanitizer: heap-use-after-free on address 0x72603898 at pc 0x0009b451 bp 0x7e9678f8 sp 0x7e9678fc
    READ of size 4 at 0x72603898 thread T0
        #0 0x9b44e in connman_service_get_nameservers:service.c:2896 (/usr/sbin/connmand+0x9b44e)
        #1 0xd485e in ts_set_nameservers:timeserver.c:387 (/usr/sbin/connmand+0xd485e)
        #2 0xd4930 in sync_next:timeserver.c:187 (/usr/sbin/connmand+0xd4930)
        #3 0xd4e6a in resolv_result:timeserver.c:141 (/usr/sbin/connmand+0xd4e6a)
        #4 0x37b5a in sort_and_return_results:gresolv.c:522 (/usr/sbin/connmand+0x37b5a)
        #5 0x385e4 in query_timeout:gresolv.c:547 (/usr/sbin/connmand+0x385e4)

    0x72603898 is located 88 bytes inside of 408-byte region [0x72603840,0x726039d8)
    freed by thread T0 here:
       #0 0x76a0d264  (/lib/libasan.so.8+0x8d264)

    previously allocated by thread T0 here:
        #0 0x76a0da6c in calloc (/lib/libasan.so.8+0x8da6c)
        #1 0x925b4 in connman_service_create (/usr/sbin/connmand+0x925b4)
        #2 0x9294c  (/usr/sbin/connmand+0x9294c)
        #3 0x9e844  (/usr/sbin/connmand+0x9e844)
        #4 0x7dda6  (/usr/sbin/connmand+0x7dda6)
        #5 0x7ea18 in connman_network_set_group (/usr/sbin/connmand+0x7ea18)
        #6 0x3da36  (/usr/sbin/connmand+0x3da36)
        #7 0x3db56  (/usr/sbin/connmand+0x3db56)
        #8 0xd057c  (/usr/sbin/connmand+0xd057c)
        #9 0xd0882  (/usr/sbin/connmand+0xd0882)
        #10 0xd28da  (/usr/sbin/connmand+0xd28da)
        #11 0xd2ca4  (/usr/sbin/connmand+0xd2ca4)

   SUMMARY: AddressSanitizer: heap-use-after-free (/usr/sbin/connmand+0x9b44e) in connman_service_get_nameservers
gerickson added a commit that referenced this issue Nov 2, 2023
This qualifies the call to '__connman_timeserver_sync' with an
'is_connected' check on the new state of the service in
'__connman_service_ipconfig_indicate_state'. In fact, this leverages
the same conditional that 'nameserver_add_all' is also subject to.
This avoids a heap-use-after-free segmentation fault that can occur
when a network service goes away following link loss or a disable of
the corresponding technology.

Without this qualification, the network service going away will invoke
'__connman_timeserver_sync' with the
'CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE' reason as it cycles from
online/ready to disconnect. As currently implemented, the timeserver
code interprets this as a request to initiate time-of-day synchroninzation
with time services using the name and time services of that disappearing
service. As a result, this has two downstream impacts:

  1. In a multi-service connectivity environment, the junior service that
     was promoted when the senior service was disconnected and was set as
     the active service for time services when 'default_changed' was
     invoked is undesireably displaced by the disconnecting service.

  2. The implication of (1) is that the time service now holds a weak
     reference to the disappearing service in 'ts_service' rather than
     the promoted and new default service. Consequentl, when 'sync_next'
     is next called by time services, it will attempt to get the name
     or time servers for the disconnecting service. Unfortunately,
     these which have gone away, as 'service_free' was invoked on the
     disconnected service after it was wound down. It is the access to
     these name or time servers on the deallocated service object that
     causes the heap-use-after-free segmentation fault.

The address sanitizer report is as follows:

    =================================================================
    ==526==ERROR: AddressSanitizer: heap-use-after-free on address 0x72603898 at pc 0x0009b451 bp 0x7e9678f8 sp 0x7e9678fc
    READ of size 4 at 0x72603898 thread T0
        #0 0x9b44e in connman_service_get_nameservers:service.c:2896 (/usr/sbin/connmand+0x9b44e)
        #1 0xd485e in ts_set_nameservers:timeserver.c:387 (/usr/sbin/connmand+0xd485e)
        #2 0xd4930 in sync_next:timeserver.c:187 (/usr/sbin/connmand+0xd4930)
        #3 0xd4e6a in resolv_result:timeserver.c:141 (/usr/sbin/connmand+0xd4e6a)
        #4 0x37b5a in sort_and_return_results:gresolv.c:522 (/usr/sbin/connmand+0x37b5a)
        #5 0x385e4 in query_timeout:gresolv.c:547 (/usr/sbin/connmand+0x385e4)

    0x72603898 is located 88 bytes inside of 408-byte region [0x72603840,0x726039d8)
    freed by thread T0 here:
       #0 0x76a0d264  (/lib/libasan.so.8+0x8d264)

    previously allocated by thread T0 here:
        #0 0x76a0da6c in calloc (/lib/libasan.so.8+0x8da6c)
        #1 0x925b4 in connman_service_create (/usr/sbin/connmand+0x925b4)
        #2 0x9294c  (/usr/sbin/connmand+0x9294c)
        #3 0x9e844  (/usr/sbin/connmand+0x9e844)
        #4 0x7dda6  (/usr/sbin/connmand+0x7dda6)
        #5 0x7ea18 in connman_network_set_group (/usr/sbin/connmand+0x7ea18)
        #6 0x3da36  (/usr/sbin/connmand+0x3da36)
        #7 0x3db56  (/usr/sbin/connmand+0x3db56)
        #8 0xd057c  (/usr/sbin/connmand+0xd057c)
        #9 0xd0882  (/usr/sbin/connmand+0xd0882)
        #10 0xd28da  (/usr/sbin/connmand+0xd28da)
        #11 0xd2ca4  (/usr/sbin/connmand+0xd2ca4)

   SUMMARY: AddressSanitizer: heap-use-after-free (/usr/sbin/connmand+0x9b44e) in connman_service_get_nameservers
gerickson added a commit that referenced this issue Nov 2, 2023
This changes the semantics of dealing with 'ts_service', the active
network service against which time-of-day synchronization is run,
from a weak (that is, simple assignment) reference to a strong (that
is, uses 'connman_service_{ref,unref}') reference.

This avoids a stale reference that can occur in failure paths
should the time service find itself 'ts_service' referencing
a disconnecting and, consequently, disappearing service.

This stale reference would otherwise lead to a heap-use-after-free
fault when the time services attempts to access the name or time
services associated with the stale service.

By promoting the weak reference to a strong reference, a potential
service deallocation is deferred until 'timeserver_stop' is invoked
and the reference to the service released.

The address sanitizer report associated with the potential heap-use-
after-free is as follows:

    =================================================================
    ==526==ERROR: AddressSanitizer: heap-use-after-free on address 0x72603898 at pc 0x0009b451 bp 0x7e9678f8 sp 0x7e9678fc
    READ of size 4 at 0x72603898 thread T0
        #0 0x9b44e in connman_service_get_nameservers:service.c:2896 (/usr/sbin/connmand+0x9b44e)
        #1 0xd485e in ts_set_nameservers:timeserver.c:387 (/usr/sbin/connmand+0xd485e)
        #2 0xd4930 in sync_next:timeserver.c:187 (/usr/sbin/connmand+0xd4930)
        #3 0xd4e6a in resolv_result:timeserver.c:141 (/usr/sbin/connmand+0xd4e6a)
        #4 0x37b5a in sort_and_return_results:gresolv.c:522 (/usr/sbin/connmand+0x37b5a)
        #5 0x385e4 in query_timeout:gresolv.c:547 (/usr/sbin/connmand+0x385e4)

    0x72603898 is located 88 bytes inside of 408-byte region [0x72603840,0x726039d8)
    freed by thread T0 here:
       #0 0x76a0d264  (/lib/libasan.so.8+0x8d264)

    previously allocated by thread T0 here:
        #0 0x76a0da6c in calloc (/lib/libasan.so.8+0x8da6c)
        #1 0x925b4 in connman_service_create (/usr/sbin/connmand+0x925b4)
        #2 0x9294c  (/usr/sbin/connmand+0x9294c)
        #3 0x9e844  (/usr/sbin/connmand+0x9e844)
        #4 0x7dda6  (/usr/sbin/connmand+0x7dda6)
        #5 0x7ea18 in connman_network_set_group (/usr/sbin/connmand+0x7ea18)
        #6 0x3da36  (/usr/sbin/connmand+0x3da36)
        #7 0x3db56  (/usr/sbin/connmand+0x3db56)
        #8 0xd057c  (/usr/sbin/connmand+0xd057c)
        #9 0xd0882  (/usr/sbin/connmand+0xd0882)
        #10 0xd28da  (/usr/sbin/connmand+0xd28da)
        #11 0xd2ca4  (/usr/sbin/connmand+0xd2ca4)

   SUMMARY: AddressSanitizer: heap-use-after-free (/usr/sbin/connmand+0x9b44e) in connman_service_get_nameservers
gerickson added a commit that referenced this issue Nov 9, 2023
This qualifies the call to '__connman_timeserver_sync' with an
'is_connected' check on the new state of the service in
'__connman_service_ipconfig_indicate_state'. In fact, this leverages
the same conditional that 'nameserver_add_all' is also subject to.
This avoids a heap-use-after-free segmentation fault that can occur
when a network service goes away following link loss or a disable of
the corresponding technology.

Without this qualification, the network service going away will invoke
'__connman_timeserver_sync' with the
'CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE' reason as it cycles from
online/ready to disconnect. As currently implemented, the timeserver
code interprets this as a request to initiate time-of-day synchroninzation
with time services using the name and time services of that disappearing
service. As a result, this has two downstream impacts:

  1. In a multi-service connectivity environment, the junior service that
     was promoted when the senior service was disconnected and was set as
     the active service for time services when 'default_changed' was
     invoked is undesireably displaced by the disconnecting service.

  2. The implication of (1) is that the time service now holds a weak
     reference to the disappearing service in 'ts_service' rather than
     the promoted and new default service. Consequentl, when 'sync_next'
     is next called by time services, it will attempt to get the name
     or time servers for the disconnecting service. Unfortunately,
     these which have gone away, as 'service_free' was invoked on the
     disconnected service after it was wound down. It is the access to
     these name or time servers on the deallocated service object that
     causes the heap-use-after-free segmentation fault.

The address sanitizer report is as follows:

    =================================================================
    ==526==ERROR: AddressSanitizer: heap-use-after-free on address 0x72603898 at pc 0x0009b451 bp 0x7e9678f8 sp 0x7e9678fc
    READ of size 4 at 0x72603898 thread T0
        #0 0x9b44e in connman_service_get_nameservers:service.c:2896 (/usr/sbin/connmand+0x9b44e)
        #1 0xd485e in ts_set_nameservers:timeserver.c:387 (/usr/sbin/connmand+0xd485e)
        #2 0xd4930 in sync_next:timeserver.c:187 (/usr/sbin/connmand+0xd4930)
        #3 0xd4e6a in resolv_result:timeserver.c:141 (/usr/sbin/connmand+0xd4e6a)
        #4 0x37b5a in sort_and_return_results:gresolv.c:522 (/usr/sbin/connmand+0x37b5a)
        #5 0x385e4 in query_timeout:gresolv.c:547 (/usr/sbin/connmand+0x385e4)

    0x72603898 is located 88 bytes inside of 408-byte region [0x72603840,0x726039d8)
    freed by thread T0 here:
       #0 0x76a0d264  (/lib/libasan.so.8+0x8d264)

    previously allocated by thread T0 here:
        #0 0x76a0da6c in calloc (/lib/libasan.so.8+0x8da6c)
        #1 0x925b4 in connman_service_create (/usr/sbin/connmand+0x925b4)
        #2 0x9294c  (/usr/sbin/connmand+0x9294c)
        #3 0x9e844  (/usr/sbin/connmand+0x9e844)
        #4 0x7dda6  (/usr/sbin/connmand+0x7dda6)
        #5 0x7ea18 in connman_network_set_group (/usr/sbin/connmand+0x7ea18)
        #6 0x3da36  (/usr/sbin/connmand+0x3da36)
        #7 0x3db56  (/usr/sbin/connmand+0x3db56)
        #8 0xd057c  (/usr/sbin/connmand+0xd057c)
        #9 0xd0882  (/usr/sbin/connmand+0xd0882)
        #10 0xd28da  (/usr/sbin/connmand+0xd28da)
        #11 0xd2ca4  (/usr/sbin/connmand+0xd2ca4)

   SUMMARY: AddressSanitizer: heap-use-after-free (/usr/sbin/connmand+0x9b44e) in connman_service_get_nameservers
gerickson added a commit that referenced this issue Nov 9, 2023
This changes the semantics of dealing with 'ts_service', the active
network service against which time-of-day synchronization is run,
from a weak (that is, simple assignment) reference to a strong (that
is, uses 'connman_service_{ref,unref}') reference.

This avoids a stale reference that can occur in failure paths
should the time service find itself 'ts_service' referencing
a disconnecting and, consequently, disappearing service.

This stale reference would otherwise lead to a heap-use-after-free
fault when the time services attempts to access the name or time
services associated with the stale service.

By promoting the weak reference to a strong reference, a potential
service deallocation is deferred until 'timeserver_stop' is invoked
and the reference to the service released.

The address sanitizer report associated with the potential heap-use-
after-free is as follows:

    =================================================================
    ==526==ERROR: AddressSanitizer: heap-use-after-free on address 0x72603898 at pc 0x0009b451 bp 0x7e9678f8 sp 0x7e9678fc
    READ of size 4 at 0x72603898 thread T0
        #0 0x9b44e in connman_service_get_nameservers:service.c:2896 (/usr/sbin/connmand+0x9b44e)
        #1 0xd485e in ts_set_nameservers:timeserver.c:387 (/usr/sbin/connmand+0xd485e)
        #2 0xd4930 in sync_next:timeserver.c:187 (/usr/sbin/connmand+0xd4930)
        #3 0xd4e6a in resolv_result:timeserver.c:141 (/usr/sbin/connmand+0xd4e6a)
        #4 0x37b5a in sort_and_return_results:gresolv.c:522 (/usr/sbin/connmand+0x37b5a)
        #5 0x385e4 in query_timeout:gresolv.c:547 (/usr/sbin/connmand+0x385e4)

    0x72603898 is located 88 bytes inside of 408-byte region [0x72603840,0x726039d8)
    freed by thread T0 here:
       #0 0x76a0d264  (/lib/libasan.so.8+0x8d264)

    previously allocated by thread T0 here:
        #0 0x76a0da6c in calloc (/lib/libasan.so.8+0x8da6c)
        #1 0x925b4 in connman_service_create (/usr/sbin/connmand+0x925b4)
        #2 0x9294c  (/usr/sbin/connmand+0x9294c)
        #3 0x9e844  (/usr/sbin/connmand+0x9e844)
        #4 0x7dda6  (/usr/sbin/connmand+0x7dda6)
        #5 0x7ea18 in connman_network_set_group (/usr/sbin/connmand+0x7ea18)
        #6 0x3da36  (/usr/sbin/connmand+0x3da36)
        #7 0x3db56  (/usr/sbin/connmand+0x3db56)
        #8 0xd057c  (/usr/sbin/connmand+0xd057c)
        #9 0xd0882  (/usr/sbin/connmand+0xd0882)
        #10 0xd28da  (/usr/sbin/connmand+0xd28da)
        #11 0xd2ca4  (/usr/sbin/connmand+0xd2ca4)

   SUMMARY: AddressSanitizer: heap-use-after-free (/usr/sbin/connmand+0x9b44e) in connman_service_get_nameservers
gerickson added a commit that referenced this issue Nov 10, 2023
This balances previously-unbalanced WISPr/portal context reference
counting associated with proxy handling that can lead to a heap-use-
after-free fault.

Prior to this change, 'proxy_callback' contained an unbalanced release
on the reference to a WISPr/portal context that could lead to its
premature destructio and a subsequent heap-use-after-free fault in
'free_wispr_routes'.

This situation could occur, due to this imbalance, when a new WISPr/portal
request for given service and IP configuration combination displaced a
prior, but unclosed and pending within 'g_web_request_get' request,
WISPr/portal context leading to its premature deletion. Later, when
the prior 'g_web_request_get' completed, either successfully or
unsuccessfully, this resulted in a different, yet balanced release on
the reference at the end of the 'wispr_portal_web_result' callback to
'g_web_request_get'. This also resulted in its (now redundant) destruction.
The heap-use-after-free actually occurred BEFORE the reference release,
in 'free_wispr_routes' since in 'wispr_portal_web_result',
'free_wispr_routes' is invoked BEFORE the reference is released.

This imbalance is addressed by balancing the reference release in
'proxy_callback' with a reference retain prior to either invoking
'connman_proxy_lookup' with the 'proxy_callback' callback or invoking
'g_idle_add' with the 'no_proxy_callback', which ultimately calls
'proxy_callback'.

The address sanitizer report associated with the potential heap-use-
after-free is as follows:

=================================================================
==2188==ERROR: AddressSanitizer: heap-use-after-free on address 0x72b0f4b4 at pc 0x000f36b5 bp 0x7eac98c8 sp 0x7eac98cc
READ of size 4 at 0x72b0f4b4 thread T0
    #0 0xf36b2 in free_wispr_routes src/wispr.c:140
    #1 0xf5fde in wispr_portal_web_result src/wispr.c:871
    #2 0x3285e in call_result_func gweb/gweb.c:465
    #3 0x3285e in received_data gweb/gweb.c:920

0x72b0f4b4 is located 100 bytes inside of 108-byte region [0x72b0f450,0x72b0f4bc)
freed by thread T0 here:
    #0 0x76abf264 in __interceptor_free /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0xf3916 in free_connman_wispr_portal_context src/wispr.c:211
    #2 0xf4706 in wispr_portal_context_unref_debug src/wispr.c:239
    #3 0xf6408 in __connman_wispr_start src/wispr.c:1101
    #4 0x9830a in __connman_service_wispr_start src/service.c:2421
    #5 0x990d2 in start_wispr_if_connected src/service.c:2375
    #6 0x9ae2c in default_changed src/service.c:2637
    #7 0x9afb6 in __connman_service_indicate_default src/service.c:7660
    #8 0x856de in set_default_gateway src/connection.c:509
    #9 0x86e3a in __connman_connection_update_gateway src/connection.c:1053
    #10 0x9b0aa in service_update_preferred_order src/service.c:7320
    #11 0x9b2d2 in service_indicate_state src/service.c:7400
    #12 0x9baaa in __connman_service_ipconfig_indicate_state src/service.c:7848
    #13 0x9ed28 in complete_online_check src/service.c:2219
    #14 0xf44fc in portal_manage_success_status src/wispr.c:516
    #15 0xf60ac in wispr_portal_web_result src/wispr.c:820
    #16 0x3285e in call_result_func gweb/gweb.c:465
    #17 0x3285e in received_data gweb/gweb.c:920

previously allocated by thread T0 here:
    #0 0x76abfa6c in __interceptor_calloc /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77

SUMMARY: AddressSanitizer: heap-use-after-free src/wispr.c:140 in free_wispr_routes
gerickson added a commit that referenced this issue Nov 10, 2023
This balances previously-unbalanced WISPr/portal context reference
counting associated with proxy handling that can lead to a heap-use-
after-free fault.

Prior to this change, 'proxy_callback' contained an unbalanced release
on the reference to a WISPr/portal context that could lead to its
premature destructio and a subsequent heap-use-after-free fault in
'free_wispr_routes'.

This situation could occur, due to this imbalance, when a new WISPr/portal
request for given service and IP configuration combination displaced a
prior, but unclosed and pending within 'g_web_request_get' request,
WISPr/portal context leading to its premature deletion. Later, when
the prior 'g_web_request_get' completed, either successfully or
unsuccessfully, this resulted in a different, yet balanced release on
the reference at the end of the 'wispr_portal_web_result' callback to
'g_web_request_get'. This also resulted in its (now redundant) destruction.
The heap-use-after-free actually occurred BEFORE the reference release,
in 'free_wispr_routes' since in 'wispr_portal_web_result',
'free_wispr_routes' is invoked BEFORE the reference is released.

This imbalance is addressed by balancing the reference release in
'proxy_callback' with a reference retain prior to either invoking
'connman_proxy_lookup' with the 'proxy_callback' callback or invoking
'g_idle_add' with the 'no_proxy_callback', which ultimately calls
'proxy_callback'.

The address sanitizer report associated with the potential heap-use-
after-free is as follows:

=================================================================
==2188==ERROR: AddressSanitizer: heap-use-after-free on address 0x72b0f4b4 at pc 0x000f36b5 bp 0x7eac98c8 sp 0x7eac98cc
READ of size 4 at 0x72b0f4b4 thread T0
    #0 0xf36b2 in free_wispr_routes src/wispr.c:140
    #1 0xf5fde in wispr_portal_web_result src/wispr.c:871
    #2 0x3285e in call_result_func gweb/gweb.c:465
    #3 0x3285e in received_data gweb/gweb.c:920

0x72b0f4b4 is located 100 bytes inside of 108-byte region [0x72b0f450,0x72b0f4bc)
freed by thread T0 here:
    #0 0x76abf264 in __interceptor_free /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0xf3916 in free_connman_wispr_portal_context src/wispr.c:211
    #2 0xf4706 in wispr_portal_context_unref_debug src/wispr.c:239
    #3 0xf6408 in __connman_wispr_start src/wispr.c:1101
    #4 0x9830a in __connman_service_wispr_start src/service.c:2421
    #5 0x990d2 in start_wispr_if_connected src/service.c:2375
    #6 0x9ae2c in default_changed src/service.c:2637
    #7 0x9afb6 in __connman_service_indicate_default src/service.c:7660
    #8 0x856de in set_default_gateway src/connection.c:509
    #9 0x86e3a in __connman_connection_update_gateway src/connection.c:1053
    #10 0x9b0aa in service_update_preferred_order src/service.c:7320
    #11 0x9b2d2 in service_indicate_state src/service.c:7400
    #12 0x9baaa in __connman_service_ipconfig_indicate_state src/service.c:7848
    #13 0x9ed28 in complete_online_check src/service.c:2219
    #14 0xf44fc in portal_manage_success_status src/wispr.c:516
    #15 0xf60ac in wispr_portal_web_result src/wispr.c:820
    #16 0x3285e in call_result_func gweb/gweb.c:465
    #17 0x3285e in received_data gweb/gweb.c:920

previously allocated by thread T0 here:
    #0 0x76abfa6c in __interceptor_calloc /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77

SUMMARY: AddressSanitizer: heap-use-after-free src/wispr.c:140 in free_wispr_routes
gerickson added a commit that referenced this issue Nov 15, 2023
This balances previously-unbalanced WISPr/portal context reference
counting associated with proxy handling that can lead to a heap-use-
after-free fault.

Prior to this change, 'proxy_callback' contained an unbalanced release
on the reference to a WISPr/portal context that could lead to its
premature destructio and a subsequent heap-use-after-free fault in
'free_wispr_routes'.

This situation could occur, due to this imbalance, when a new WISPr/portal
request for given service and IP configuration combination displaced a
prior, but unclosed and pending within 'g_web_request_get' request,
WISPr/portal context leading to its premature deletion. Later, when
the prior 'g_web_request_get' completed, either successfully or
unsuccessfully, this resulted in a different, yet balanced release on
the reference at the end of the 'wispr_portal_web_result' callback to
'g_web_request_get'. This also resulted in its (now redundant) destruction.
The heap-use-after-free actually occurred BEFORE the reference release,
in 'free_wispr_routes' since in 'wispr_portal_web_result',
'free_wispr_routes' is invoked BEFORE the reference is released.

This imbalance is addressed by balancing the reference release in
'proxy_callback' with a reference retain prior to either invoking
'connman_proxy_lookup' with the 'proxy_callback' callback or invoking
'g_idle_add' with the 'no_proxy_callback', which ultimately calls
'proxy_callback'.

The address sanitizer report associated with the potential heap-use-
after-free is as follows:

=================================================================
==2188==ERROR: AddressSanitizer: heap-use-after-free on address 0x72b0f4b4 at pc 0x000f36b5 bp 0x7eac98c8 sp 0x7eac98cc
READ of size 4 at 0x72b0f4b4 thread T0
    #0 0xf36b2 in free_wispr_routes src/wispr.c:140
    #1 0xf5fde in wispr_portal_web_result src/wispr.c:871
    #2 0x3285e in call_result_func gweb/gweb.c:465
    #3 0x3285e in received_data gweb/gweb.c:920

0x72b0f4b4 is located 100 bytes inside of 108-byte region [0x72b0f450,0x72b0f4bc)
freed by thread T0 here:
    #0 0x76abf264 in __interceptor_free /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0xf3916 in free_connman_wispr_portal_context src/wispr.c:211
    #2 0xf4706 in wispr_portal_context_unref_debug src/wispr.c:239
    #3 0xf6408 in __connman_wispr_start src/wispr.c:1101
    #4 0x9830a in __connman_service_wispr_start src/service.c:2421
    #5 0x990d2 in start_wispr_if_connected src/service.c:2375
    #6 0x9ae2c in default_changed src/service.c:2637
    #7 0x9afb6 in __connman_service_indicate_default src/service.c:7660
    #8 0x856de in set_default_gateway src/connection.c:509
    #9 0x86e3a in __connman_connection_update_gateway src/connection.c:1053
    #10 0x9b0aa in service_update_preferred_order src/service.c:7320
    #11 0x9b2d2 in service_indicate_state src/service.c:7400
    #12 0x9baaa in __connman_service_ipconfig_indicate_state src/service.c:7848
    #13 0x9ed28 in complete_online_check src/service.c:2219
    #14 0xf44fc in portal_manage_success_status src/wispr.c:516
    #15 0xf60ac in wispr_portal_web_result src/wispr.c:820
    #16 0x3285e in call_result_func gweb/gweb.c:465
    #17 0x3285e in received_data gweb/gweb.c:920

previously allocated by thread T0 here:
    #0 0x76abfa6c in __interceptor_calloc /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77

SUMMARY: AddressSanitizer: heap-use-after-free src/wispr.c:140 in free_wispr_routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-related issue help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant