Segfault on d5ff39d "tree: Actually delete any deleted nodes"

Segfault on d5ff39d "tree: Actually delete any deleted nodes"

[Valters Jansons]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Hi,

I am seeing a segfault during tests (src/test-as, src/test-country and
src/test-signature) for all Ubuntu LTS versions. This only occurs with
the latest commit (d5ff39d) applied. Looking at the change, the node
still seems to be used after dereference, but I am not fully sure of
the way you have planned out the deletion, therefore I am not offering
a patch/resolution.

>From gdb I am seeing:

Program received signal SIGSEGV, Segmentation fault.
loc_network_tree_delete_nodes (tree=0x55555555a370) at src/network.c:1045
1045            tree->root->deleted = 0;

Best regards,
Valters Jansons

Re: Segfault on d5ff39d "tree: Actually delete any deleted nodes"

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

Hello Valters,

Thanks for reviewing my changes. This was indeed a thing that I did observe as well and fixed it right here:

  https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=0e894966750e89d1b83c6ad90f37945d4d349118

The root node got deleted and lots of the code just relied on it being there. Especially when the tree was empty.

-Michael

> On 17 Jul 2023, at 19:35, Valters Jansons <valter.jansons(a)gmail.com> wrote:
> 
> Hi,
> 
> I am seeing a segfault during tests (src/test-as, src/test-country and
> src/test-signature) for all Ubuntu LTS versions. This only occurs with
> the latest commit (d5ff39d) applied. Looking at the change, the node
> still seems to be used after dereference, but I am not fully sure of
> the way you have planned out the deletion, therefore I am not offering
> a patch/resolution.
> 
> From gdb I am seeing:
> 
> Program received signal SIGSEGV, Segmentation fault.
> loc_network_tree_delete_nodes (tree=0x55555555a370) at src/network.c:1045
> 1045            tree->root->deleted = 0;
> 
> Best regards,
> Valters Jansons

Re: Segfault on d5ff39d "tree: Actually delete any deleted nodes"

[Valters Jansons]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Mon, Jul 17, 2023 at 10:09 PM Michael Tremer
<michael.tremer(a)ipfire.org> wrote:
> Thanks for reviewing my changes. This was indeed a thing that I did observe as well and fixed it right here:
>
>   https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=0e894966750e89d1b83c6ad90f37945d4d349118
>
> The root node got deleted and lots of the code just relied on it being there. Especially when the tree was empty.

I suspected you would already be aware of the expectations that were
being broken, so wanted to highlight it. Shame on me for not
double-checking whether you had already resolved it before sending the
message!

Once again: thank you for the changes!

-Valters

Re: Segfault on d5ff39d "tree: Actually delete any deleted nodes"

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

Hello Valters,

> On 17 Jul 2023, at 20:15, Valters Jansons <valter.jansons(a)gmail.com> wrote:
> 
> On Mon, Jul 17, 2023 at 10:09 PM Michael Tremer
> <michael.tremer(a)ipfire.org> wrote:
>> Thanks for reviewing my changes. This was indeed a thing that I did observe as well and fixed it right here:
>> 
>>  https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=0e894966750e89d1b83c6ad90f37945d4d349118
>> 
>> The root node got deleted and lots of the code just relied on it being there. Especially when the tree was empty.
> 
> I suspected you would already be aware of the expectations that were
> being broken, so wanted to highlight it. Shame on me for not
> double-checking whether you had already resolved it before sending the
> message!

No, I like it when code is double-checked.

I did notice myself but after I pushed my first set of changes. I knew there was some work left to do, but deleting the root node was something I thought I had covered when I did not.

So, please keep the feedback coming. It will help us all to have a better version of libloc.

Best,
-Michael

> Once again: thank you for the changes!
> 
> -Valters