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.

Sign up for GitHub

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

Jump to bottom

GLib Warning: GError set over the top of a previous GError #9

Closed
gerickson opened this issue Oct 31, 2023 · 2 comments
Closed

GLib Warning: GError set over the top of a previous GError #9

gerickson opened this issue Oct 31, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@gerickson
Copy link
Contributor

gerickson commented Oct 31, 2023 •
edited
Loading

When launching connmand with:

# CONNMAN_RESOLV_DEBUG=1 CONNMAN_WEB_DEBUG=1 /usr/sbin/connmand -n -d -W nl80211

GLib reports the following error:

connmand[538]: Connection Manager version 1.42
connmand[538]: src/dbus.c:__connman_dbus_init() 
connmand[538]: src/main.c:parse_config() parsing main.conf

(connmand:538): GLib-WARNING **: 10:34:04.783: GError set over the top of a previous GError or uninitialized memory.
This indicates a bug in someone's code. You must ensure an error is NULL before it's set.
The overwriting error message was: Key file does not have key ?ResolvConf? in group ?General?
@gerickson gerickson added the bug Something isn't working label Oct 31, 2023
@gerickson
Copy link
Contributor Author

This seems to be related to #4.

@gerickson gerickson mentioned this issue Nov 1, 2023
Merged
@gerickson
Copy link
Contributor Author

Closed with #19.

gerickson added a commit that referenced this issue Nov 2, 2023
@gerickson
Qualify request to time-of-day sync with connected check.
2720564 
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
@gerickson
timeserver: Convert ts_service from a weak to a strong reference
e2ba092 
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
@gerickson
Qualify request to time-of-day sync with connected check.
3d64001 
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
@gerickson
timeserver: Convert ts_service from a weak to a strong reference
7e74685 
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
@gerickson @holtmann
Qualify request to time-of-day sync with connected check.
b2ebc58 
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
@gerickson @holtmann
timeserver: Convert ts_service from a weak to a strong reference
1300787 
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
@gerickson
wispr: Address unbalanced reference counting with proxy handling.
d92d036 
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
@gerickson
wispr: Address unbalanced reference counting with proxy handling.
9d9187d 
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
@gerickson @holtmann
wispr: Address unbalanced reference counting with proxy handling.
26aa2c1 
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
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@gerickson