apparent bug about check_free_strict

apparent bug about check_free_strict

[Toomas Soome]

hi!

I did enable illumos kernel memory allocation/free checks (kmem_alloc/kmem_free) and apparently I did find something interesting.

The warning is:
/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'
/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8612 e_ddi_retire_device() warn: passing freed memory 'dip'
/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8621 e_ddi_retire_device() warn: passing freed memory ‘dip'

The code for first error about pdip is:

  8572          pdip = ddi_get_parent(dip);
  8573          ndi_hold_devi(pdip);
  8574     8575          /*
  8576           * Run devfs_clean() in case dip has no constraints and is
  8577           * not in use, so is retireable but there are dv_nodes holding
  8578           * ref-count on the dip. Note that devfs_clean() always returns
  8579           * success.
  8580           */
  8581          devnm = kmem_alloc(MAXNAMELEN + 1, KM_SLEEP);
  8582          (void) ddi_deviname(dip, devnm);
  8583          (void) devfs_clean(pdip, devnm + 1, DV_CLEAN_FORCE);
  8584          kmem_free(devnm, MAXNAMELEN + 1);
  8585     8586          ndi_devi_enter(pdip);

We get this error about pdip with devfs_clean(), but apparently the ‘freed’ state is set with ndi_hold_devi(pdip) call; of course the call itself is not the quilty one, but the construct is — as soon as I either comment the ndi_hold_devi() out *or* if I move it down before devfs_clean(), then the error disappears.

Therefore, it appears that code segment such as:

var = f();
g(var);

is causing state of var to be set ‘freed’ and check_free_strict.c is ending up spitting out the warning about passing freed memory with next function call.


now the next warning is about code:

  8609          constraint = 1; /* assume constraints allow retire */
  8610          (void) e_ddi_retire_notify(dip, &constraint);
  8611          if (!is_leaf_node(dip)) {
  8612                  ndi_devi_enter(dip);
  8613                  ddi_walk_devs(ddi_get_child(dip), e_ddi_retire_notify,
  8614                      &constraint);
  8615                  ndi_devi_exit(dip);
  8616          }
  8617
  8618          /*
  8619           * Now finalize the retire
  8620           */
  8621          (void) e_ddi_retire_finalize(dip, &constraint);
  8622          if (!is_leaf_node(dip)) {
  8623                  ndi_devi_enter(dip);
  8624                  ddi_walk_devs(ddi_get_child(dip), e_ddi_retire_finalize,
  8625                      &constraint);
  8626                  ndi_devi_exit(dip);
  8627          }

Here we do get warning about ndi_devi_enter(), but if I replace dip in is_leaf_node() by NULL, we do not get any more warnings about ‘dip’.

PS: the line number differences with git is because my branch has other change fixing memory leak discovered by smatch:D

rgds,
toomas

Re: apparent bug about check_free_strict

[Dan Carpenter]

On Mon, Nov 18, 2024 at 01:55:30PM +0200, Toomas Soome wrote:
> hi!
> 
> I did enable illumos kernel memory allocation/free checks (kmem_alloc/kmem_free) and apparently I did find something interesting.
> 
> The warning is:
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8612 e_ddi_retire_device() warn: passing freed memory 'dip'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8621 e_ddi_retire_device() warn: passing freed memory ‘dip'
> 
> The code for first error about pdip is:
> 
>   8572          pdip = ddi_get_parent(dip);
>   8573          ndi_hold_devi(pdip);
>   8574     8575          /*
>   8576           * Run devfs_clean() in case dip has no constraints and is
>   8577           * not in use, so is retireable but there are dv_nodes holding
>   8578           * ref-count on the dip. Note that devfs_clean() always returns
>   8579           * success.
>   8580           */
>   8581          devnm = kmem_alloc(MAXNAMELEN + 1, KM_SLEEP);
>   8582          (void) ddi_deviname(dip, devnm);
>   8583          (void) devfs_clean(pdip, devnm + 1, DV_CLEAN_FORCE);
>   8584          kmem_free(devnm, MAXNAMELEN + 1);
>   8585     8586          ndi_devi_enter(pdip);
> 
> We get this error about pdip with devfs_clean(), but apparently the ‘freed
> state is set with ndi_hold_devi(pdip) call; of course the call itself is not
> the quilty one, but the construct is — as soon as I either comment the
> ndi_hold_devi() out *or* if I move it down before devfs_clean(), then
> the error disappears.
> 
> Therefore, it appears that code segment such as:
> 
> var = f();
> g(var);
> 
> is causing state of var to be set ‘freed’ and check_free_strict.c is ending up
> spitting out the warning about passing freed memory with next function call.
> 

I don't see anything in ndi_hold_devi() which would mark "pdip" as freed.

I don't know how to run Smatch on this file...  Could you re-run Smatch with the
--debug="free" option and save the output to a file?  Maybe send that output
along with the whole file or run it against the lates git so I can match the
line numbers up.

Are you using the cross function DB?  If so then you could do:

	smdb.py return_states ndi_hold_devi | grep -i free
	smdb.py return_states is_leaf_node | grep -i free

Somewhere there is a function marking code as freed incorrectly.  But there
the check_free_strict.c file doesn't really have a long list of free functions.
They're at the top:
https://github.com/error27/smatch/blob/master/check_free_strict.c#L50

> 
> now the next warning is about code:
> 
>   8609          constraint = 1; /* assume constraints allow retire */
>   8610          (void) e_ddi_retire_notify(dip, &constraint);
>   8611          if (!is_leaf_node(dip)) {
>   8612                  ndi_devi_enter(dip);
>   8613                  ddi_walk_devs(ddi_get_child(dip), e_ddi_retire_notify,
>   8614                      &constraint);
>   8615                  ndi_devi_exit(dip);
>   8616          }
>   8617
>   8618          /*
>   8619           * Now finalize the retire
>   8620           */
>   8621          (void) e_ddi_retire_finalize(dip, &constraint);
>   8622          if (!is_leaf_node(dip)) {
>   8623                  ndi_devi_enter(dip);
>   8624                  ddi_walk_devs(ddi_get_child(dip), e_ddi_retire_finalize,
>   8625                      &constraint);
>   8626                  ndi_devi_exit(dip);
>   8627          }
> 
> Here we do get warning about ndi_devi_enter(), but if I replace dip in is_leaf_node() by NULL, we do not get any more warnings about ‘dip’.
> 

There are two reasons why that is.  1)  Smatch thinks is_leaf_node() is freeing
the parameter.  2)  If you call is_leaf_node(NULL) then it leads to a crash and
the check_free_strict.c code only tries to warn about use after frees which are
reachable.

> PS: the line number differences with git is because my branch has other change
> fixing memory leak discovered by smatch:D

Fantastic.  :)

regards,
dan carpenter

Re: apparent bug about check_free_strict

[Toomas Soome]

> On 18. Nov 2024, at 14:52, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> On Mon, Nov 18, 2024 at 01:55:30PM +0200, Toomas Soome wrote:
>> hi!
>> 
>> I did enable illumos kernel memory allocation/free checks (kmem_alloc/kmem_free) and apparently I did find something interesting.
>> 
>> The warning is:
>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'
>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8612 e_ddi_retire_device() warn: passing freed memory 'dip'
>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8621 e_ddi_retire_device() warn: passing freed memory ‘dip'
>> 
>> The code for first error about pdip is:
>> 
>>  8572          pdip = ddi_get_parent(dip);
>>  8573          ndi_hold_devi(pdip);
>>  8574     8575          /*
>>  8576           * Run devfs_clean() in case dip has no constraints and is
>>  8577           * not in use, so is retireable but there are dv_nodes holding
>>  8578           * ref-count on the dip. Note that devfs_clean() always returns
>>  8579           * success.
>>  8580           */
>>  8581          devnm = kmem_alloc(MAXNAMELEN + 1, KM_SLEEP);
>>  8582          (void) ddi_deviname(dip, devnm);
>>  8583          (void) devfs_clean(pdip, devnm + 1, DV_CLEAN_FORCE);
>>  8584          kmem_free(devnm, MAXNAMELEN + 1);
>>  8585     8586          ndi_devi_enter(pdip);
>> 
>> We get this error about pdip with devfs_clean(), but apparently the ‘freed
>> state is set with ndi_hold_devi(pdip) call; of course the call itself is not
>> the quilty one, but the construct is — as soon as I either comment the
>> ndi_hold_devi() out *or* if I move it down before devfs_clean(), then
>> the error disappears.
>> 
>> Therefore, it appears that code segment such as:
>> 
>> var = f();
>> g(var);
>> 
>> is causing state of var to be set ‘freed’ and check_free_strict.c is ending up
>> spitting out the warning about passing freed memory with next function call.
>> 
> 
> I don't see anything in ndi_hold_devi() which would mark "pdip" as freed.
> 


As I wrote, I do not think it is really about the function itself, it is about the sequence — when I moved the ndi_hold_devi() down just before the ‘pdip’ was actually called, then this warning did disappear.

> I don't know how to run Smatch on this file...  Could you re-run Smatch with the
> --debug="free" option and save the output to a file?  Maybe send that output
> along with the whole file or run it against the lates git so I can match the
> line numbers up.
> 

I guess it is a bit more complicated because you will also need the illumos specific update to check_free_strict.

The command in usr/src/uts/intel/genunix is run as:

/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch --debug=free -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -m64 -mtune=opteron -Ui386 -U__i386 -fno-strict-aliasing -fno-unit-at-a-time -fno-optimize-sibling-calls -O2 -D_ASM_INLINES -ffreestanding -mno-red-zone -mno-mmx -mno-sse -msave-args -Wall -Wextra -g -gdwarf-2 -std=gnu99 -msave-args -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Winline -Wno-unused -Wno-empty-body -p=illumos_kernel --disable=uninitialized,check_check_deref -Wno-vla -Wno-one-bit-signed-bitfield -Wno-external-function-has-definition -Wno-old-style-definition -Wno-strict-prototypes --fatal-checks --timeout=0 --disable=index_overflow --disable=signed,all_func_returns -Wno-unused-variable -Wno-unused-value -Wno-unused-function -Wno-parentheses -Wno-maybe-uninitialized -Wno-clobbered -Wno-empty-body -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -fno-reorder-blocks-and-partition -fno-aggressive-loop-optimizations --param=max-inline-insns-single=450 -fno-shrink-wrap -mindirect-branch=thunk-extern -mindirect-branch-register -fno-asynchronous-unwind-tables -fstack-protector-strong -fno-eliminate-unused-debug-symbols -fno-eliminate-unused-debug-types -D_KERNEL -ffreestanding -D_SYSCALL32 -D_SYSCALL32_IMPL -D_ELF64 -D_DDI_STRICT -Dsun -D__sun -D__SVR4 -DOPTERON_ERRATUM_88 -DOPTERON_ERRATUM_91 -DOPTERON_ERRATUM_93 -DOPTERON_ERRATUM_95 -DOPTERON_ERRATUM_99 -DOPTERON_ERRATUM_100 -DOPTERON_ERRATUM_101 -DOPTERON_ERRATUM_108 -DOPTERON_ERRATUM_109 -DOPTERON_ERRATUM_121 -DOPTERON_ERRATUM_122 -DOPTERON_ERRATUM_123 -DOPTERON_ERRATUM_131 -DOPTERON_WORKAROUND_6336786 -DOPTERON_ERRATUM_147 -DOPTERON_ERRATUM_172 -DOPTERON_ERRATUM_298 -DOPTERON_ERRATUM_721 -I../../intel -nostdinc -I../../common -I/code/illumos-gate/usr/src/common -I/code/illumos-gate/usr/src/uts/common/fs/zfs -I../../i86pc -c -o /tmp/cw.GCaq5P/cwICaO5P.o ../../common/os/devcfg.c -mcmodel=kernel


I did put the samples of files into http://132-104-190-90.sta.estpak.ee/smatch/, I still need to clean up a bit my smatch repo, that will take a bit more time.

thanks,
toomas

> Are you using the cross function DB?  If so then you could do:
> 
> smdb.py return_states ndi_hold_devi | grep -i free
> smdb.py return_states is_leaf_node | grep -i free
> 
> Somewhere there is a function marking code as freed incorrectly.  But there
> the check_free_strict.c file doesn't really have a long list of free functions.
> They're at the top:
> https://github.com/error27/smatch/blob/master/check_free_strict.c#L50
> 
>> 
>> now the next warning is about code:
>> 
>>  8609          constraint = 1; /* assume constraints allow retire */
>>  8610          (void) e_ddi_retire_notify(dip, &constraint);
>>  8611          if (!is_leaf_node(dip)) {
>>  8612                  ndi_devi_enter(dip);
>>  8613                  ddi_walk_devs(ddi_get_child(dip), e_ddi_retire_notify,
>>  8614                      &constraint);
>>  8615                  ndi_devi_exit(dip);
>>  8616          }
>>  8617
>>  8618          /*
>>  8619           * Now finalize the retire
>>  8620           */
>>  8621          (void) e_ddi_retire_finalize(dip, &constraint);
>>  8622          if (!is_leaf_node(dip)) {
>>  8623                  ndi_devi_enter(dip);
>>  8624                  ddi_walk_devs(ddi_get_child(dip), e_ddi_retire_finalize,
>>  8625                      &constraint);
>>  8626                  ndi_devi_exit(dip);
>>  8627          }
>> 
>> Here we do get warning about ndi_devi_enter(), but if I replace dip in is_leaf_node() by NULL, we do not get any more warnings about ‘dip’.
>> 
> 
> There are two reasons why that is.  1)  Smatch thinks is_leaf_node() is freeing
> the parameter.  2)  If you call is_leaf_node(NULL) then it leads to a crash and
> the check_free_strict.c code only tries to warn about use after frees which are
> reachable.
> 
>> PS: the line number differences with git is because my branch has other change
>> fixing memory leak discovered by smatch:D
> 
> Fantastic.  :)
> 
> regards,
> dan carpenter
> 

Re: apparent bug about check_free_strict

[Dan Carpenter]

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

On Mon, Nov 18, 2024 at 03:28:57PM +0200, Toomas Soome wrote:
> 
> 
> > On 18. Nov 2024, at 14:52, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > 
> > On Mon, Nov 18, 2024 at 01:55:30PM +0200, Toomas Soome wrote:
> >> hi!
> >> 
> >> I did enable illumos kernel memory allocation/free checks (kmem_alloc/kmem_free) and apparently I did find something interesting.
> >> 
> >> The warning is:
> >> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'
> >> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8612 e_ddi_retire_device() warn: passing freed memory 'dip'
> >> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8621 e_ddi_retire_device() warn: passing freed memory ‘dip'
> >> 
> >> The code for first error about pdip is:
> >> 
> >>  8572          pdip = ddi_get_parent(dip);
> >>  8573          ndi_hold_devi(pdip);
> >>  8574     8575          /*
> >>  8576           * Run devfs_clean() in case dip has no constraints and is
> >>  8577           * not in use, so is retireable but there are dv_nodes holding
> >>  8578           * ref-count on the dip. Note that devfs_clean() always returns
> >>  8579           * success.
> >>  8580           */
> >>  8581          devnm = kmem_alloc(MAXNAMELEN + 1, KM_SLEEP);
> >>  8582          (void) ddi_deviname(dip, devnm);
> >>  8583          (void) devfs_clean(pdip, devnm + 1, DV_CLEAN_FORCE);
> >>  8584          kmem_free(devnm, MAXNAMELEN + 1);
> >>  8585     8586          ndi_devi_enter(pdip);
> >> 
> >> We get this error about pdip with devfs_clean(), but apparently the ‘freed
> >> state is set with ndi_hold_devi(pdip) call; of course the call itself is not
> >> the quilty one, but the construct is — as soon as I either comment the
> >> ndi_hold_devi() out *or* if I move it down before devfs_clean(), then
> >> the error disappears.
> >> 
> >> Therefore, it appears that code segment such as:
> >> 
> >> var = f();
> >> g(var);
> >> 
> >> is causing state of var to be set ‘freed’ and check_free_strict.c is ending up
> >> spitting out the warning about passing freed memory with next function call.
> >> 
> > 
> > I don't see anything in ndi_hold_devi() which would mark "pdip" as freed.
> > 
> 
> 
> As I wrote, I do not think it is really about the function itself, it is about
> the sequence — when I moved the ndi_hold_devi() down just before the ‘pdip’ was actually called, then this warning did disappear.
> 
> > I don't know how to run Smatch on this file...  Could you re-run Smatch with the
> > --debug="free" option and save the output to a file?  Maybe send that output
> > along with the whole file or run it against the lates git so I can match the
> > line numbers up.
> > 
> 
> I guess it is a bit more complicated because you will also need the illumos specific update to check_free_strict.
> 
> The command in usr/src/uts/intel/genunix is run as:
> 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch --debug=free -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -m64 -mtune=opteron -Ui386 -U__i386 -fno-strict-aliasing -fno-unit-at-a-time -fno-optimize-sibling-calls -O2 -D_ASM_INLINES -ffreestanding -mno-red-zone -mno-mmx -mno-sse -msave-args -Wall -Wextra -g -gdwarf-2 -std=gnu99 -msave-args -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Winline -Wno-unused -Wno-empty-body -p=illumos_kernel --disable=uninitialized,check_check_deref -Wno-vla -Wno-one-bit-signed-bitfield -Wno-external-function-has-definition -Wno-old-style-definition -Wno-strict-prototypes --fatal-checks --timeout=0 --disable=index_overflow --disable=signed,all_func_returns -Wno-unused-variable -Wno-unused-value -Wno-unused-function -Wno-parentheses -Wno-maybe-uninitialized -Wno-clobbered -Wno-empty-body -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -fno-reorder-blocks-and-partition -fno-aggressive-loop-optimizations --param=max-inline-insns-single=450 -fno-shrink-wrap -mindirect-branch=thunk-extern -mindirect-branch-register -fno-asynchronous-unwind-tables -fstack-protector-strong -fno-eliminate-unused-debug-symbols -fno-eliminate-unused-debug-types -D_KERNEL -ffreestanding -D_SYSCALL32 -D_SYSCALL32_IMPL -D_ELF64 -D_DDI_STRICT -Dsun -D__sun -D__SVR4 -DOPTERON_ERRATUM_88 -DOPTERON_ERRATUM_91 -DOPTERON_ERRATUM_93 -DOPTERON_ERRATUM_95 -DOPTERON_ERRATUM_99 -DOPTERON_ERRATUM_100 -DOPTERON_ERRATUM_101 -DOPTERON_ERRATUM_108 -DOPTERON_ERRATUM_109 -DOPTERON_ERRATUM_121 -DOPTERON_ERRATUM_122 -DOPTERON_ERRATUM_123 -DOPTERON_ERRATUM_131 -DOPTERON_WORKAROUND_6336786 -DOPTERON_ERRATUM_147 -DOPTERON_ERRATUM_172 -DOPTERON_ERRATUM_298 -DOPTERON_ERRATUM_721 -I../../intel -nostdinc -I../../common -I/code/illumos-gate/usr/src/common -I/code/illumos-gate/usr/src/uts/common/fs/zfs -I../../i86pc -c -o /tmp/cw.GCaq5P/cwICaO5P.o ../../common/os/devcfg.c -mcmodel=kernel
> 
> 
> I did put the samples of files into http://132-104-190-90.sta.estpak.ee/smatch/,
> I still need to clean up a bit my smatch repo, that will take a bit more time.
> 

There's something weird going on.  I don't see any problem with the changes you
have made to check_free_strict.c.  I started to just merge your changes but then
I realized I don't know who to give authorship credit etc but I'd already pushed
whatever changes I had on my end so I just made the situation worse.

You are testing with the latest Smatch right?

Anyway, I've attached the diff to the latest code below with some debugging.
Apply the diff to the lastest Smatch and apply the code-diff to the devcfg.c
and then re-run Smatch.

regards,
dan carpenter



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 2083 bytes --]

diff --git a/check_free_strict.c b/check_free_strict.c
index 2838054e476d..be92b33776b7 100644
--- a/check_free_strict.c
+++ b/check_free_strict.c
@@ -47,6 +47,10 @@ struct func_info {
 	param_key_hook *call_back;
 };
 
+static struct func_info illumos_func_table[] = {
+	{ "kmem_free", PARAM_FREED, 0, "$" },
+};
+
 static struct func_info func_table[] = {
 	{ "free", PARAM_FREED, 0, "$" },
 	{ "kfree", PARAM_FREED, 0, "$" },
@@ -460,6 +464,8 @@ static void match_free(struct expression *expr, const char *name, struct symbol
 
 	track_freed_param(arg, &freed);
 	call_free_call_backs_name_sym(name, sym);
+	if (local_debug)
+		sm_msg("%s: expr='%s' name='%s'", __func__, expr_to_str(expr), name);
 	set_state_expr(my_id, arg, &freed);
 }
 
@@ -505,6 +511,8 @@ static void match___skb_pad(struct expression *expr, const char *name, struct sy
 
 	skb = get_argument_from_call_expr(expr->args, 0);
 	track_freed_param(skb, state);
+	if (local_debug)
+		sm_msg("%s: expr='%s' name='%s'", __func__, expr_to_str(expr), name);
 	set_state_expr(my_id, skb, state);
 }
 
@@ -565,6 +573,8 @@ free:
 
 static void set_param_freed(struct expression *expr, int param, char *key, char *value)
 {
+	if (local_debug)
+		sm_msg("%s: expr='%s' param=%d key='%s'", __func__, expr_to_str(expr), param, key);
 	set_param_helper(expr, param, key, value, &freed);
 }
 
@@ -652,17 +662,28 @@ static void match_untracked(struct expression *call, int param)
 
 void check_free_strict(int id)
 {
-	struct func_info *info;
+	struct func_info *info, *table;
 	param_key_hook *cb;
+	size_t array_size;
 	int i;
 
 	my_id = id;
 
-	if (option_project != PROJ_KERNEL)
+	switch (option_project) {
+	case PROJ_KERNEL:
+		table = func_table;
+		array_size = ARRAY_SIZE(func_table);
+		break;
+	case PROJ_ILLUMOS_KERNEL:
+		table = illumos_func_table;
+		array_size = ARRAY_SIZE(illumos_func_table);
+		break;
+	default:
 		return;
+	}
 
-	for (i = 0; i < ARRAY_SIZE(func_table); i++) {
-		info = &func_table[i];
+	for (i = 0; i < array_size; i++) {
+		info = &table[i];
 
 		insert_string(&handled, info->name);
 

[-- Attachment #3: code-diff --]
[-- Type: text/plain, Size: 825 bytes --]

diff --git a/usr/src/uts/common/os/devcfg.c b/usr/src/uts/common/os/devcfg.c
index cc37b39ec5fb..b74c8bec9782 100644
--- a/usr/src/uts/common/os/devcfg.c
+++ b/usr/src/uts/common/os/devcfg.c
@@ -8532,6 +8539,7 @@ e_ddi_retire_finalize(dev_info_t *dip, void *arg)
  * cons_array is a NULL terminated array of node paths for
  * which constraints have already been applied.
  */
+#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
 int
 e_ddi_retire_device(char *path, char **cons_array)
 {
@@ -8563,7 +8571,9 @@ e_ddi_retire_device(char *path, char **cons_array)
 	RIO_DEBUG((CE_NOTE, "Retire device: found dip = %p.", (void *)dip));
 
 	pdip = ddi_get_parent(dip);
+	__smatch_local_debug_on();
 	ndi_hold_devi(pdip);
+	__smatch_local_debug_off();
 
 	/*
 	 * Run devfs_clean() in case dip has no constraints and is

Re: apparent bug about check_free_strict

[Toomas Soome]

Oops, Mail did create it using HTML, resending in plain text.



> On 18. Nov 2024, at 23:17, Toomas Soome <tsoome@me.com> wrote:
> 
> 
> 
>> On 18. Nov 2024, at 17:27, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>> 
>> On Mon, Nov 18, 2024 at 03:28:57PM +0200, Toomas Soome wrote:
>>> 
>>> 
>>>> On 18. Nov 2024, at 14:52, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>>> 
>>>> On Mon, Nov 18, 2024 at 01:55:30PM +0200, Toomas Soome wrote:
>>>>> hi!
>>>>> 
>>>>> I did enable illumos kernel memory allocation/free checks (kmem_alloc/kmem_free) and apparently I did find something interesting.
>>>>> 
>>>>> The warning is:
>>>>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'
>>>>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8612 e_ddi_retire_device() warn: passing freed memory 'dip'
>>>>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8621 e_ddi_retire_device() warn: passing freed memory ‘dip'
>>>>> 
>>>>> The code for first error about pdip is:
>>>>> 
>>>>> 8572          pdip = ddi_get_parent(dip);
>>>>> 8573          ndi_hold_devi(pdip);
>>>>> 8574     8575          /*
>>>>> 8576           * Run devfs_clean() in case dip has no constraints and is
>>>>> 8577           * not in use, so is retireable but there are dv_nodes holding
>>>>> 8578           * ref-count on the dip. Note that devfs_clean() always returns
>>>>> 8579           * success.
>>>>> 8580           */
>>>>> 8581          devnm = kmem_alloc(MAXNAMELEN + 1, KM_SLEEP);
>>>>> 8582          (void) ddi_deviname(dip, devnm);
>>>>> 8583          (void) devfs_clean(pdip, devnm + 1, DV_CLEAN_FORCE);
>>>>> 8584          kmem_free(devnm, MAXNAMELEN + 1);
>>>>> 8585     8586          ndi_devi_enter(pdip);
>>>>> 
>>>>> We get this error about pdip with devfs_clean(), but apparently the ‘freed
>>>>> state is set with ndi_hold_devi(pdip) call; of course the call itself is not
>>>>> the quilty one, but the construct is — as soon as I either comment the
>>>>> ndi_hold_devi() out *or* if I move it down before devfs_clean(), then
>>>>> the error disappears.
>>>>> 
>>>>> Therefore, it appears that code segment such as:
>>>>> 
>>>>> var = f();
>>>>> g(var);
>>>>> 
>>>>> is causing state of var to be set ‘freed’ and check_free_strict.c is ending up
>>>>> spitting out the warning about passing freed memory with next function call.
>>>>> 
>>>> 
>>>> I don't see anything in ndi_hold_devi() which would mark "pdip" as freed.
>>>> 
>>> 
>>> 
>>> As I wrote, I do not think it is really about the function itself, it is about
>>> the sequence — when I moved the ndi_hold_devi() down just before the ‘pdip’ was actually called, then this warning did disappear.
>>> 
>>>> I don't know how to run Smatch on this file...  Could you re-run Smatch with the
>>>> --debug="free" option and save the output to a file?  Maybe send that output
>>>> along with the whole file or run it against the lates git so I can match the
>>>> line numbers up.
>>>> 
>>> 
>>> I guess it is a bit more complicated because you will also need the illumos specific update to check_free_strict.
>>> 
>>> The command in usr/src/uts/intel/genunix is run as:
>>> 
>>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch --debug=free -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -m64 -mtune=opteron -Ui386 -U__i386 -fno-strict-aliasing -fno-unit-at-a-time -fno-optimize-sibling-calls -O2 -D_ASM_INLINES -ffreestanding -mno-red-zone -mno-mmx -mno-sse -msave-args -Wall -Wextra -g -gdwarf-2 -std=gnu99 -msave-args -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Winline -Wno-unused -Wno-empty-body -p=illumos_kernel --disable=uninitialized,check_check_deref -Wno-vla -Wno-one-bit-signed-bitfield -Wno-external-function-has-definition -Wno-old-style-definition -Wno-strict-prototypes --fatal-checks --timeout=0 --disable=index_overflow --disable=signed,all_func_returns -Wno-unused-variable -Wno-unused-value -Wno-unused-function -Wno-parentheses -Wno-maybe-uninitialized -Wno-clobbered -Wno-empty-body -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -fno-reorder-blocks-and-partition -fno-aggressive-loop-optimizations --param=max-inline-insns-single=450 -fno-shrink-wrap -mindirect-branch=thunk-extern -mindirect-branch-register -fno-asynchronous-unwind-tables -fstack-protector-strong -fno-eliminate-unused-debug-symbols -fno-eliminate-unused-debug-types -D_KERNEL -ffreestanding -D_SYSCALL32 -D_SYSCALL32_IMPL -D_ELF64 -D_DDI_STRICT -Dsun -D__sun -D__SVR4 -DOPTERON_ERRATUM_88 -DOPTERON_ERRATUM_91 -DOPTERON_ERRATUM_93 -DOPTERON_ERRATUM_95 -DOPTERON_ERRATUM_99 -DOPTERON_ERRATUM_100 -DOPTERON_ERRATUM_101 -DOPTERON_ERRATUM_108 -DOPTERON_ERRATUM_109 -DOPTERON_ERRATUM_121 -DOPTERON_ERRATUM_122 -DOPTERON_ERRATUM_123 -DOPTERON_ERRATUM_131 -DOPTERON_WORKAROUND_6336786 -DOPTERON_ERRATUM_147 -DOPTERON_ERRATUM_172 -DOPTERON_ERRATUM_298 -DOPTERON_ERRATUM_721 -I../../intel -nostdinc -I../../common -I/code/illumos-gate/usr/src/common -I/code/illumos-gate/usr/src/uts/common/fs/zfs -I../../i86pc -c -o /tmp/cw.GCaq5P/cwICaO5P.o ../../common/os/devcfg.c -mcmodel=kernel
>>> 
>>> 
>>> I did put the samples of files into http://132-104-190-90.sta.estpak.ee/smatch/,
>>> I still need to clean up a bit my smatch repo, that will take a bit more time.
>>> 
>> 
>> There's something weird going on.  I don't see any problem with the changes you
>> have made to check_free_strict.c.  I started to just merge your changes but then
>> I realized I don't know who to give authorship credit etc but I'd already pushed
>> whatever changes I had on my end so I just made the situation worse.
>> 
> 
> :D the kmem_alloc/kmem_free related bits are done by me. But we can sort later what we could and should upstream and what maybe not.
> 
>> You are testing with the latest Smatch right?
> 
> 
> Yes. My current head of master branch is:
> 
> commit 9a427ca57dc8a8b47d021f5f772ac164842bd996 (upstream/master, master)
> Author: Dan Carpenter <dan.carpenter@linaro.org>
> Date:   Thu Nov 14 23:12:46 2024 +0300
> 
>     db: fix a NULL dereference in get_param()
> 
>> 
>> Anyway, I've attached the diff to the latest code below with some debugging.
>> Apply the diff to the lastest Smatch and apply the code-diff to the devcfg.c
>> and then re-run Smatch.
>> 
> 
> Now this is fun. With devcfg.c changes in place, the warnings disappeared. So I removed 'if (local_debug)’ from check_free_strict.c and, I think we have something to point to:
> 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() set_param_freed: expr='ndi_hold_devi(pdip)' param=0 key='$'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() set_state new [check_free_strict] 'pdip' freed
> ../../common/os/devcfg.c:8573 e_ddi_retire_device() merge [check_free_strict] 'pdip' freed(L 8573) + undefined(L 8573) => merged (freed, undefined, merged)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() __set_sm new [check_free_strict] pdip fffff7ffe9523b90 = 'merged' [merged] (freed, undefined, merged)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'
> 
> and the same for ‘dip’:
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8611 e_ddi_retire_device() set_param_freed: expr='is_leaf_node(dip)' param=0 key='$'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8611 e_ddi_retire_device() set_state new [check_free_strict] 'dip’ freed
> 
> and we will have:/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8612 e_ddi_retire_device() warn: passing freed memory 'dip'
> 
> So, set_param_freed() does register ‘pdip’ as freed, and this callback is set up as
> 
> select_return_states_hook(PARAM_FREED, &set_param_freed);
> 
> thanks,
> toomas


Hi!

I got some time to get back to this issue - yep, it is still present with current smatch. So I did use a bit of dtrace - I print out user space stack when we do enter set_param_freed(), and to verify the expression, I have this update:
 static void set_param_freed(struct expression *expr, int param, char *key, char *value)
 {
+       sm_msg("%s: expr='%s' param=%d key='%s'", __func__, expr_to_str(expr), param, key);
        set_param_helper(expr, param, key, value, &freed);
 }

it allows me to print out the expression with dtrace; the dtrace command line is there:

sudo dtrace -n 'pid$target::set_param_freed:entry { self->p = 1; ustack(); } pid$target::set_param_freed:return {self->p = 0;} pid$target::expr_to_str:return /self->p/ { printf("%s\n", copyinstr(arg1));}' -c '/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -m64 -mtune=opteron -Ui386 -U__i386 -fno-strict-aliasing -fno-unit-at-a-time -fno-optimize-sibling-calls -O2 -D_ASM_INLINES -ffreestanding -mno-red-zone -mno-mmx -mno-sse -msave-args -Wall -Wextra -g -gdwarf-4 -gstrict-dwarf -std=gnu99 -msave-args -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unused-parameter -Wno-missing-field-initializers -Winline -Wno-unused -Wno-empty-body -p=illumos_kernel --disable=uninitialized,check_check_deref -Wno-vla -Wno-one-bit-signed-bitfield -Wno-external-function-has-definition -Wno-old-style-definition -Wno-strict-prototypes --fatal-checks --timeout=0 --disable=index_overflow --disable=signed,all_func_returns -Wno-unused-variable -Wno-unused-value -Wno-unused-function -Wno-parentheses -Wno-maybe-uninitialized -Wno-clobbered -Wno-empty-body -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -fno-reorder-blocks-and-partition -fno-aggressive-loop-optimizations --param=max-inline-insns-single=450 -fno-shrink-wrap -mindirect-branch=thunk-extern -mindirect-branch-register -fno-asynchronous-unwind-tables -fstack-protector-strong -fno-eliminate-unused-debug-symbols -fno-eliminate-unused-debug-types -D_KERNEL -ffreestanding -D_SYSCALL32 -D_SYSCALL32_IMPL -D_ELF64 -D_DDI_STRICT -Dsun -D__sun -D__SVR4 -DOPTERON_ERRATUM_88 -DOPTERON_ERRATUM_91 -DOPTERON_ERRATUM_93 -DOPTERON_ERRATUM_95 -DOPTERON_ERRATUM_99 -DOPTERON_ERRATUM_100 -DOPTERON_ERRATUM_101 -DOPTERON_ERRATUM_108 -DOPTERON_ERRATUM_109 -DOPTERON_ERRATUM_121 -DOPTERON_ERRATUM_122 -DOPTERON_ERRATUM_123 -DOPTERON_ERRATUM_131 -DOPTERON_WORKAROUND_6336786 -DOPTERON_ERRATUM_147 -DOPTERON_ERRATUM_172 -DOPTERON_ERRATUM_298 -DOPTERON_ERRATUM_721 -I../../intel -nostdinc -I../../common -I/code/illumos-gate/usr/src/common -I/code/illumos-gate/usr/src/uts/common/fs/zfs -I../../i86pc -c -o /tmp/cwMia4d4.o ../../common/os/devcfg.c -mcmodel=kernel’


and  for ‘pdip’ above, it did tell the user stack:

 10  87183            set_param_freed:entry 
              smatch`set_param_freed
              smatch`call_db_return_callback+0x98
              smatch`db_return_states_callback+0x34c
              libsqlite3.so.3.50.4`sqlite3_exec+0x559
              smatch`sql_exec+0x1a1
              smatch`sql_select_return_states+0x15f
              smatch`db_return_states+0x9a
              smatch`db_return_states_call+0x59
              smatch`match_function_call+0x2e
              smatch`pass_expr_to_client+0x1f
              smatch`__pass_to_client+0xc8
              smatch`split_call+0x138
              smatch`__split_expr+0x4c5
              smatch`__split_stmt+0x335
              smatch`split_compound+0xca
              smatch`__split_stmt+0x346
              smatch`parse_fn_statements+0x24
              smatch`split_function+0x1c2
              smatch`split_c_file_functions+0x1d2
              smatch`smatch+0x17c

 10  87185               expr_to_str:return ndi_hold_devi(pdip)

 10  87185               expr_to_str:return pdip

Apparently we get ‘set_param_freed()’ called to mark “pdip” freed from sql query, but we do not build sqllite db on disk - is it in memory db?

rgds,
toomas

Re: apparent bug about check_free_strict

[Dan Carpenter]

Hi Toomas,

I've recently re-written the check for use after frees.  Could you
retest?  I have another fix for use after free which I'm going to
send tomorrow hopefully...

regards,
dan carpenter

Re: apparent bug about check_free_strict

[Toomas Soome]

> On 24. Nov 2025, at 16:46, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> Hi Toomas,
> 
> I've recently re-written the check for use after frees.  Could you
> retest?  I have another fix for use after free which I'm going to
> send tomorrow hopefully...
> 
> regards,
> dan carpenter
> 
> 

I do have the latest HEAD and yes, it is still there. It’s not a single isolated case, on whole illumos kernel tree its 10 cases.

It *may* be related, but I have stepped on on some curious SIGSEGV case too:

tsoome@balrog:/code/illumos-gate/usr/src/cmd/diskinfo$ mdb core 
Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
> ::stack -t
struct expression * cast_expression+0xf((struct expression *)NULL, (struct symbol *)fffff7ffe9334610)
void fake_return_assignment+0x74((struct db_callback_info *)fffff7ffffdf0520, (int)405, (int)ffffffff, (char *)1a31938, (char *)1a31e38)
int db_assign_return_states_callback+0x2ef((void *)fffff7ffffdf0520, (int)6, (char **)1a324e8, (char **)1a324b8)
int libsqlite3.so.3.50.4`sqlite3_exec+0x559()
void sql_exec+0x1b6((struct sqlite3 *)1a0ab98, (int (*)())4e543f, (void *)fffff7ffffdf0520, (const char *)fffff7ffffdefd70)
void sql_select_return_states+0x396((const char *)5a2908, (struct expression *)fffff7ffec1d6960, (int (*)())4e543f, (void *)fffff7ffffdf0520)
int db_return_states_assign+0x89((struct expression *)fffff7ffebff5e70)
void match_assign_call+0x70((struct expression *)fffff7ffebff5e70)
void __pass_to_client+0x57((void *)fffff7ffebff5e70, (enum hook_type)CALL_ASSIGNMENT_HOOK)
void parse_assignment+0x2a0((struct expression *)fffff7ffebff5e70, (_Bool)0)
void __split_expr+0x317((struct expression *)fffff7ffebff5e70)
void __split_expr+0x644((struct expression *)fffff7ffec1d6780)
void parse_assignment+0x134((struct expression *)fffff7ffebff5ce0, (_Bool)0)
void __split_expr+0x317((struct expression *)fffff7ffebff5ce0)
void __split_expr+0x578((struct expression *)fffff7ffec1d6780)
void __split_stmt+0x3e2((struct statement *)fffff7ffea4849f8)
void __split_stmt+0x4c8((struct statement *)fffff7ffea4849a0)
void __split_stmt+0x56b((struct statement *)fffff7ffea484948)
void __split_stmt+0x4c8((struct statement *)fffff7ffea4848f0)
void __split_stmt+0x56b((struct statement *)fffff7ffea484898)
void __split_stmt+0x442((struct statement *)fffff7ffea4828a0)
void handle_pre_loop+0x17f((struct statement *)fffff7ffea482848)
void __split_stmt+0x886((struct statement *)fffff7ffea482848)
void __split_stmt+0x442((struct statement *)fffff7ffea482060)
void split_function+0x113((struct symbol *)fffff7ffeb3c0d30)
void smatch+0x3e3((struct string_list *)fffff7ffef2c1590)
int main+0x18f((int)33, (char **)fffff7ffffdf1ea8)
_start_crt+0x87()
_start+0x18()
> 

there we also do get some data from sql but for some reason we end up passing NULL pointer to cast_expression where it is not expected. I’m trying to figure out how to spot the location from source code the smatch is processing there, some hints about it would be much welcome;)

rgds,
toomas

Re: apparent bug about check_free_strict

[Toomas Soome]

Sorry, forgot to make it plain text again:D


Ok, i did pull your latest update, and the problem is still there. I actually got very graphical example:

/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../smb_share_doorclnt.c:274 smb_share_count() warn: passing freed memory 'dec_ctx' (line 272)
/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../smb_share_doorclnt.c:279 smb_share_count() warn: passing freed memory 'dec_ctx' (line 272)

now, the code in question is:

246 smb_share_count(void)
247 {
248         door_arg_t *arg;
249         smb_dr_ctx_t *dec_ctx;
250         smb_dr_ctx_t *enc_ctx;
251         uint32_t num_shares;
252         int rc;
253 
254         if ((arg = smb_share_door_clnt_enter()) == NULL)
255                 return (-1);
256 
257         enc_ctx = smb_dr_encode_start(arg->data_ptr, SMB_SHARE_DSIZE);
258         smb_dr_put_uint32(enc_ctx, SMB_SHROP_NUM_SHARES);
259 
260         rc = smb_dr_encode_finish(enc_ctx, (unsigned int *)&arg->data_size);
261         if (rc != 0) {
262                 smb_share_door_clnt_exit(arg, B_FALSE, "encode");
263                 return (-1);
264         }
265 
266         if (smb_share_door_call(smb_share_dfd, arg) < 0) {
267                 smb_share_door_clnt_exit(arg, B_TRUE, "door call");
268                 return (-1);
269         }
270 
271         dec_ctx = smb_dr_decode_start(arg->data_ptr, arg->data_size);
272         if (dec_ctx == NULL || smb_share_dchk(dec_ctx) != 0) {
273                 if (dec_ctx != NULL)
274                         (void) smb_dr_decode_finish(dec_ctx);
275                 smb_share_door_clnt_exit(arg, B_FALSE, "decode");
276                 return (-1);
277         }
278 
279         num_shares = smb_dr_get_uint32(dec_ctx);
280         if (smb_dr_decode_finish(dec_ctx) != 0) {
281                 smb_share_door_clnt_exit(arg, B_FALSE, "decode");
282                 return (-1);
283         }
284 
285         smb_share_door_clnt_exit(arg, B_FALSE, NULL);
286         return (num_shares);
287 }

Both warning locations do have preceding “if" statements… 

Other cases (and including the case mentioned below), the warning is referring to some function which does not free the pointer, so it has to be about some sort of context processing but so far I havent been able to find any logical reason to get such warning;)

Anyhow, the example above hints there is something still a miss in this detection logic.

thanks,
toomas


> On 24. Nov 2025, at 17:30, Toomas Soome <tsoome@me.com> wrote:
> 
> 
> 
>> On 24. Nov 2025, at 16:46, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>> 
>> Hi Toomas,
>> 
>> I've recently re-written the check for use after frees.  Could you
>> retest?  I have another fix for use after free which I'm going to
>> send tomorrow hopefully...
>> 
>> regards,
>> dan carpenter
>> 
>> 
> 
> I do have the latest HEAD and yes, it is still there. It’s not a single isolated case, on whole illumos kernel tree its 10 cases.
> 
> It *may* be related, but I have stepped on on some curious SIGSEGV case too:
> 
> tsoome@balrog:/code/illumos-gate/usr/src/cmd/diskinfo$ mdb core 
> Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
>> ::stack -t
> struct expression * cast_expression+0xf((struct expression *)NULL, (struct symbol *)fffff7ffe9334610)
> void fake_return_assignment+0x74((struct db_callback_info *)fffff7ffffdf0520, (int)405, (int)ffffffff, (char *)1a31938, (char *)1a31e38)
> int db_assign_return_states_callback+0x2ef((void *)fffff7ffffdf0520, (int)6, (char **)1a324e8, (char **)1a324b8)
> int libsqlite3.so.3.50.4`sqlite3_exec+0x559()
> void sql_exec+0x1b6((struct sqlite3 *)1a0ab98, (int (*)())4e543f, (void *)fffff7ffffdf0520, (const char *)fffff7ffffdefd70)
> void sql_select_return_states+0x396((const char *)5a2908, (struct expression *)fffff7ffec1d6960, (int (*)())4e543f, (void *)fffff7ffffdf0520)
> int db_return_states_assign+0x89((struct expression *)fffff7ffebff5e70)
> void match_assign_call+0x70((struct expression *)fffff7ffebff5e70)
> void __pass_to_client+0x57((void *)fffff7ffebff5e70, (enum hook_type)CALL_ASSIGNMENT_HOOK)
> void parse_assignment+0x2a0((struct expression *)fffff7ffebff5e70, (_Bool)0)
> void __split_expr+0x317((struct expression *)fffff7ffebff5e70)
> void __split_expr+0x644((struct expression *)fffff7ffec1d6780)
> void parse_assignment+0x134((struct expression *)fffff7ffebff5ce0, (_Bool)0)
> void __split_expr+0x317((struct expression *)fffff7ffebff5ce0)
> void __split_expr+0x578((struct expression *)fffff7ffec1d6780)
> void __split_stmt+0x3e2((struct statement *)fffff7ffea4849f8)
> void __split_stmt+0x4c8((struct statement *)fffff7ffea4849a0)
> void __split_stmt+0x56b((struct statement *)fffff7ffea484948)
> void __split_stmt+0x4c8((struct statement *)fffff7ffea4848f0)
> void __split_stmt+0x56b((struct statement *)fffff7ffea484898)
> void __split_stmt+0x442((struct statement *)fffff7ffea4828a0)
> void handle_pre_loop+0x17f((struct statement *)fffff7ffea482848)
> void __split_stmt+0x886((struct statement *)fffff7ffea482848)
> void __split_stmt+0x442((struct statement *)fffff7ffea482060)
> void split_function+0x113((struct symbol *)fffff7ffeb3c0d30)
> void smatch+0x3e3((struct string_list *)fffff7ffef2c1590)
> int main+0x18f((int)33, (char **)fffff7ffffdf1ea8)
> _start_crt+0x87()
> _start+0x18()
>> 
> 
> there we also do get some data from sql but for some reason we end up passing NULL pointer to cast_expression where it is not expected. I’m trying to figure out how to spot the location from source code the smatch is processing there, some hints about it would be much welcome;)
> 
> rgds,
> toomas

Re: apparent bug about check_free_strict

[Dan Carpenter]

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

It feels like this email I'm responding to is recent because it's from
Nov 18.  But actually it's from Nov 18 last year...  Sorry for that.

I'm going to have to start again from scratch in some ways because I've
forgotten everything.  I've attached my reproducer script (which doesn't
reproduce anything).  There was a mistake in the reproducer script that
you sent me because the smatch options (particularly -p=illumos_kernel)
need to come before the GCC options...

I guessed the dir to be in and I had to touch the
usr/src/uts/i86pc/sys/priv_names.h file because I don't have a copy of
that but generally the reproducer seems fine.  Without the
-p=illumos_kernel then "ddi_err(DER_PANIC, ..." calls were not parsed
correctly so it generated some false positives but after I fixed that
then there was no output.

(I removed the --debug=free for now).

On Mon, Nov 18, 2024 at 11:17:12PM +0200, Toomas Soome wrote:
> commit 9a427ca57dc8a8b47d021f5f772ac164842bd996 (upstream/master, master)
> Author: Dan Carpenter <dan.carpenter@linaro.org>
> Date:   Thu Nov 14 23:12:46 2024 +0300
> 
>     db: fix a NULL dereference in get_param()
> 
> > 
> > Anyway, I've attached the diff to the latest code below with some debugging.
> > Apply the diff to the lastest Smatch and apply the code-diff to the devcfg.c
> > and then re-run Smatch.
> > 
> 
> Now this is fun. With devcfg.c changes in place, the warnings disappeared. So I removed 'if (local_debug)’ from check_free_strict.c and, I think we have something to point to:
> 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() set_param_freed: expr='ndi_hold_devi(pdip)' param=0 key='$'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() set_state new [check_free_strict] 'pdip' freed
> ../../common/os/devcfg.c:8573 e_ddi_retire_device() merge [check_free_strict] 'pdip' freed(L 8573) + undefined(L 8573) => merged (freed, undefined, merged)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() __set_sm new [check_free_strict] pdip fffff7ffe9523b90 = 'merged' [merged] (freed, undefined, merged)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'

What this says is that ndi_hold_devi() is setting pdip to free.  I would
have thought you have a cross function database somewhere.  Your dtrace
output also suggests that it's coming from the DB.  But you've explained
later in the thread that you don't.  It's possible that the ndi_hold_devi()
function is being parsed inline.  One way you can prevent that is by
adding a bunch of semi-colons to the function since it won't inline
functions with a lot of lines of code.

diff --git a/usr/src/uts/common/os/devcfg.c b/usr/src/uts/common/os/devcfg.c
index 787d49468e1c..f7fda31d5c23 100644
--- a/usr/src/uts/common/os/devcfg.c
+++ b/usr/src/uts/common/os/devcfg.c
@@ -1877,6 +1877,8 @@ ddi_remove_child(dev_info_t *dip, int dummy)
 void
 ndi_hold_devi(dev_info_t *dip)
 {
+	;;;;;;;;;;;;;
+
 	mutex_enter(&DEVI(dip)->devi_lock);
 	ASSERT(DEVI(dip)->devi_ref >= 0);
 	DEVI(dip)->devi_ref++;

But I don't see a kmem_free() in the ndi_hold_devi() function and I
also don't see ndi_hold_devi() listed in:
http://132-104-190-90.sta.estpak.ee/smatch/smatch.debug.log
And in my reproducer, Smatch doesn't mark it as a free function.

The fact that you're getting segfaults suggests there is memory
corruption happening somewhere.  That could explain it.  I tried
adding a valgrind before the smatch in my check script.
"valgrind ~/progs/smatch/$RELEASE/smatch --full-path..." but
valgrind didn't find anything.

Maybe you could try again with this diff (but with the correct
include path name obviously).

diff --git a/usr/src/uts/common/os/devcfg.c b/usr/src/uts/common/os/devcfg.c
index 787d49468e1c..a577bcbd49ee 100644
--- a/usr/src/uts/common/os/devcfg.c
+++ b/usr/src/uts/common/os/devcfg.c
@@ -8565,6 +8565,7 @@ e_ddi_retire_finalize(dev_info_t *dip, void *arg)
  * cons_array is a NULL terminated array of node paths for
  * which constraints have already been applied.
  */
+#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
 int
 e_ddi_retire_device(char *path, char **cons_array)
 {
@@ -8596,7 +8597,9 @@ e_ddi_retire_device(char *path, char **cons_array)
 	RIO_DEBUG((CE_NOTE, "Retire device: found dip = %p.", (void *)dip));
 
 	pdip = ddi_get_parent(dip);
+	__smatch_debug_db_on();
 	ndi_hold_devi(pdip);
+	__smatch_debug_db_off();
 
 	/*
 	 * Run devfs_clean() in case dip has no constraints and is

I don't see any corrupted output when I run it, but maybe you will
get different results.

regards,
dan carpenter


[-- Attachment #2: check.sh --]
[-- Type: application/x-sh, Size: 2227 bytes --]

Re: apparent bug about check_free_strict

[Dan Carpenter]

On Tue, Nov 25, 2025 at 03:37:08PM +0200, Toomas Soome wrote:
> 
> Ok, i did pull your latest update, and the problem is still there. I actually got very graphical example:
> 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../smb_share_doorclnt.c:274 smb_share_count() warn: passing freed memory 'dec_ctx' (line 272)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../smb_share_doorclnt.c:279 smb_share_count() warn: passing freed memory 'dec_ctx' (line 272)
> 

These warnings are almost certainly caused because Smatch thinks
smb_share_dchk() is freeing the pointer.  It generates two warnings
because the if and else paths are handled separately.

It's possible this is a Makefile bug...  Could you do a make clean
and rebuild Smatch?

regards,
dan carpenter

Re: apparent bug about check_free_strict

[Toomas Soome]

And another interesting case:

smatch is complaining about about ‘pptr’ but we do free ‘ptr’. 

/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: adm_kef_util.c:1243 filter_mechlist() error: dereferencing freed memory 'pptr' (line 1242)

1225 filter_mechlist(mechlist_t **pmechlist, const char *mech)
1226 {
1227         int             cnt = 0;
1228         mechlist_t      *ptr, *pptr;
1229         boolean_t       mech_present = B_FALSE;
1230  
1231         ptr = pptr = *pmechlist;
1232  
1233         while (ptr != NULL) {
1234                 if (strncmp(ptr->name, mech, sizeof (mech_name_t)) == 0) {
1235                         mech_present = B_TRUE;
1236                         if (ptr == *pmechlist) {
1237                                 pptr = *pmechlist = ptr->next;
1238                                 free(ptr);
1239                                 ptr = pptr;
1240                         } else {
1241                                 pptr->next = ptr->next;
1242                                 free(ptr);
1243                                 ptr = pptr->next;
1244                         }
1245                 } else {
1246                         pptr = ptr;
1247                         ptr = ptr->next;
1248                         cnt++;
1249                 }
1250         }
1251  
1252         /* Only one entry is present */
1253         if (cnt == 0)
1254                 *pmechlist = NULL;
1255  
1256         return (mech_present);
1257 }

thanks,
toomas

> On 25. Nov 2025, at 15:38, Toomas Soome <tsoome@me.com> wrote:
> 
> Sorry, forgot to make it plain text again:D
> 
> 
> Ok, i did pull your latest update, and the problem is still there. I actually got very graphical example:
> 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../smb_share_doorclnt.c:274 smb_share_count() warn: passing freed memory 'dec_ctx' (line 272)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../smb_share_doorclnt.c:279 smb_share_count() warn: passing freed memory 'dec_ctx' (line 272)
> 
> now, the code in question is:
> 
> 246 smb_share_count(void)
> 247 {
> 248         door_arg_t *arg;
> 249         smb_dr_ctx_t *dec_ctx;
> 250         smb_dr_ctx_t *enc_ctx;
> 251         uint32_t num_shares;
> 252         int rc;
> 253 
> 254         if ((arg = smb_share_door_clnt_enter()) == NULL)
> 255                 return (-1);
> 256 
> 257         enc_ctx = smb_dr_encode_start(arg->data_ptr, SMB_SHARE_DSIZE);
> 258         smb_dr_put_uint32(enc_ctx, SMB_SHROP_NUM_SHARES);
> 259 
> 260         rc = smb_dr_encode_finish(enc_ctx, (unsigned int *)&arg->data_size);
> 261         if (rc != 0) {
> 262                 smb_share_door_clnt_exit(arg, B_FALSE, "encode");
> 263                 return (-1);
> 264         }
> 265 
> 266         if (smb_share_door_call(smb_share_dfd, arg) < 0) {
> 267                 smb_share_door_clnt_exit(arg, B_TRUE, "door call");
> 268                 return (-1);
> 269         }
> 270 
> 271         dec_ctx = smb_dr_decode_start(arg->data_ptr, arg->data_size);
> 272         if (dec_ctx == NULL || smb_share_dchk(dec_ctx) != 0) {
> 273                 if (dec_ctx != NULL)
> 274                         (void) smb_dr_decode_finish(dec_ctx);
> 275                 smb_share_door_clnt_exit(arg, B_FALSE, "decode");
> 276                 return (-1);
> 277         }
> 278 
> 279         num_shares = smb_dr_get_uint32(dec_ctx);
> 280         if (smb_dr_decode_finish(dec_ctx) != 0) {
> 281                 smb_share_door_clnt_exit(arg, B_FALSE, "decode");
> 282                 return (-1);
> 283         }
> 284 
> 285         smb_share_door_clnt_exit(arg, B_FALSE, NULL);
> 286         return (num_shares);
> 287 }
> 
> Both warning locations do have preceding “if" statements… 
> 
> Other cases (and including the case mentioned below), the warning is referring to some function which does not free the pointer, so it has to be about some sort of context processing but so far I havent been able to find any logical reason to get such warning;)
> 
> Anyhow, the example above hints there is something still a miss in this detection logic.
> 
> thanks,
> toomas
> 
> 
>> On 24. Nov 2025, at 17:30, Toomas Soome <tsoome@me.com> wrote:
>> 
>> 
>> 
>>> On 24. Nov 2025, at 16:46, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>> 
>>> Hi Toomas,
>>> 
>>> I've recently re-written the check for use after frees.  Could you
>>> retest?  I have another fix for use after free which I'm going to
>>> send tomorrow hopefully...
>>> 
>>> regards,
>>> dan carpenter
>>> 
>>> 
>> 
>> I do have the latest HEAD and yes, it is still there. It’s not a single isolated case, on whole illumos kernel tree its 10 cases.
>> 
>> It *may* be related, but I have stepped on on some curious SIGSEGV case too:
>> 
>> tsoome@balrog:/code/illumos-gate/usr/src/cmd/diskinfo$ mdb core 
>> Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
>>> ::stack -t
>> struct expression * cast_expression+0xf((struct expression *)NULL, (struct symbol *)fffff7ffe9334610)
>> void fake_return_assignment+0x74((struct db_callback_info *)fffff7ffffdf0520, (int)405, (int)ffffffff, (char *)1a31938, (char *)1a31e38)
>> int db_assign_return_states_callback+0x2ef((void *)fffff7ffffdf0520, (int)6, (char **)1a324e8, (char **)1a324b8)
>> int libsqlite3.so.3.50.4`sqlite3_exec+0x559()
>> void sql_exec+0x1b6((struct sqlite3 *)1a0ab98, (int (*)())4e543f, (void *)fffff7ffffdf0520, (const char *)fffff7ffffdefd70)
>> void sql_select_return_states+0x396((const char *)5a2908, (struct expression *)fffff7ffec1d6960, (int (*)())4e543f, (void *)fffff7ffffdf0520)
>> int db_return_states_assign+0x89((struct expression *)fffff7ffebff5e70)
>> void match_assign_call+0x70((struct expression *)fffff7ffebff5e70)
>> void __pass_to_client+0x57((void *)fffff7ffebff5e70, (enum hook_type)CALL_ASSIGNMENT_HOOK)
>> void parse_assignment+0x2a0((struct expression *)fffff7ffebff5e70, (_Bool)0)
>> void __split_expr+0x317((struct expression *)fffff7ffebff5e70)
>> void __split_expr+0x644((struct expression *)fffff7ffec1d6780)
>> void parse_assignment+0x134((struct expression *)fffff7ffebff5ce0, (_Bool)0)
>> void __split_expr+0x317((struct expression *)fffff7ffebff5ce0)
>> void __split_expr+0x578((struct expression *)fffff7ffec1d6780)
>> void __split_stmt+0x3e2((struct statement *)fffff7ffea4849f8)
>> void __split_stmt+0x4c8((struct statement *)fffff7ffea4849a0)
>> void __split_stmt+0x56b((struct statement *)fffff7ffea484948)
>> void __split_stmt+0x4c8((struct statement *)fffff7ffea4848f0)
>> void __split_stmt+0x56b((struct statement *)fffff7ffea484898)
>> void __split_stmt+0x442((struct statement *)fffff7ffea4828a0)
>> void handle_pre_loop+0x17f((struct statement *)fffff7ffea482848)
>> void __split_stmt+0x886((struct statement *)fffff7ffea482848)
>> void __split_stmt+0x442((struct statement *)fffff7ffea482060)
>> void split_function+0x113((struct symbol *)fffff7ffeb3c0d30)
>> void smatch+0x3e3((struct string_list *)fffff7ffef2c1590)
>> int main+0x18f((int)33, (char **)fffff7ffffdf1ea8)
>> _start_crt+0x87()
>> _start+0x18()
>>> 
>> 
>> there we also do get some data from sql but for some reason we end up passing NULL pointer to cast_expression where it is not expected. I’m trying to figure out how to spot the location from source code the smatch is processing there, some hints about it would be much welcome;)
>> 
>> rgds,
>> toomas
> 

Re: apparent bug about check_free_strict

[Dan Carpenter]

On Tue, Nov 25, 2025 at 04:28:03PM +0200, Toomas Soome wrote:
> And another interesting case:
> 
> smatch is complaining about about ‘pptr’ but we do free ‘ptr’. 
> 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: adm_kef_util.c:1243 filter_mechlist() error: dereferencing freed memory 'pptr' (line 1242)
> 
> 1225 filter_mechlist(mechlist_t **pmechlist, const char *mech)
> 1226 {
> 1227         int             cnt = 0;
> 1228         mechlist_t      *ptr, *pptr;
> 1229         boolean_t       mech_present = B_FALSE;
> 1230  
> 1231         ptr = pptr = *pmechlist;
> 1232  
> 1233         while (ptr != NULL) {
> 1234                 if (strncmp(ptr->name, mech, sizeof (mech_name_t)) == 0) {
> 1235                         mech_present = B_TRUE;
> 1236                         if (ptr == *pmechlist) {
> 1237                                 pptr = *pmechlist = ptr->next;
> 1238                                 free(ptr);
> 1239                                 ptr = pptr;
> 1240                         } else {
> 1241                                 pptr->next = ptr->next;
> 1242                                 free(ptr);
> 1243                                 ptr = pptr->next;

This one is explainable...  Smatch is crap at loops, and only parses the
loop one time.  It might look like Smatch parses loops but it's all
hacks and special cases.

So, in this case, instead of seeing that "this is the second iteration
through the loop", Smatch says "this is dead code, but all of our other
assumptions are probably correct including that "ptr = pptr = *pmechlist".

So when we free "ptr" we're also freeing "pptr".

I've known the correct way to handle loops for over ten years now and
I partially wrote the code ten years ago.  But I've never wanted to do
it because it will slow everything down a lot.  It's quite a bit of
work as well, but mostly it was the slow down that was the issue.
But I think I'm going to try to make Smatch work better on other
projects outside the kernel so adding more and more loop hacks will
become less feasible and I will care less about slow downs so I
have decided I am going to do this work soon.

Basically you just parse every function twice and you store the next
iteration states for every loop.  Then you parse the functions again
and merge in the next iteration states.  It's a 2x slow down in
parsing.  I already have the --two-passes option but I haven't looked
at the output in a while...

regards,
dan carpenter

Re: apparent bug about check_free_strict

[Toomas Soome]

> On 25. Nov 2025, at 16:50, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> On Tue, Nov 25, 2025 at 04:28:03PM +0200, Toomas Soome wrote:
>> And another interesting case:
>> 
>> smatch is complaining about about ‘pptr’ but we do free ‘ptr’. 
>> 
>> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: adm_kef_util.c:1243 filter_mechlist() error: dereferencing freed memory 'pptr' (line 1242)
>> 
>> 1225 filter_mechlist(mechlist_t **pmechlist, const char *mech)
>> 1226 {
>> 1227         int             cnt = 0;
>> 1228         mechlist_t      *ptr, *pptr;
>> 1229         boolean_t       mech_present = B_FALSE;
>> 1230  
>> 1231         ptr = pptr = *pmechlist;
>> 1232  
>> 1233         while (ptr != NULL) {
>> 1234                 if (strncmp(ptr->name, mech, sizeof (mech_name_t)) == 0) {
>> 1235                         mech_present = B_TRUE;
>> 1236                         if (ptr == *pmechlist) {
>> 1237                                 pptr = *pmechlist = ptr->next;
>> 1238                                 free(ptr);
>> 1239                                 ptr = pptr;
>> 1240                         } else {
>> 1241                                 pptr->next = ptr->next;
>> 1242                                 free(ptr);
>> 1243                                 ptr = pptr->next;
> 
> This one is explainable...  Smatch is crap at loops, and only parses the
> loop one time.  It might look like Smatch parses loops but it's all
> hacks and special cases.
> 
> So, in this case, instead of seeing that "this is the second iteration
> through the loop", Smatch says "this is dead code, but all of our other
> assumptions are probably correct including that "ptr = pptr = *pmechlist".
> 
> So when we free "ptr" we're also freeing "pptr".
> 
> I've known the correct way to handle loops for over ten years now and
> I partially wrote the code ten years ago.  But I've never wanted to do
> it because it will slow everything down a lot.  It's quite a bit of
> work as well, but mostly it was the slow down that was the issue.
> But I think I'm going to try to make Smatch work better on other
> projects outside the kernel so adding more and more loop hacks will
> become less feasible and I will care less about slow downs so I
> have decided I am going to do this work soon.
> 
> Basically you just parse every function twice and you store the next
> iteration states for every loop.  Then you parse the functions again
> and merge in the next iteration states.  It's a 2x slow down in
> parsing.  I already have the --two-passes option but I haven't looked
> at the output in a while...
> 
> regards,
> dan carpenter

I see.

For positive side, the current smatch has been able to detect many bugs the previous versions had missed - its definitely good progress. While there are some issues, we can disable checks on such cases. Only problem is that where previously —disable=check_free_strict did work, —disable=check_free does not seem to:)

thanks,
toomas

Re: apparent bug about check_free_strict

[Oleg Drokin]

On Tue, 2025-11-25 at 17:04 +0200, Toomas Soome wrote:
> For positive side, the current smatch has been able to detect many
> bugs the previous versions had missed - its definitely good progress.
> While there are some issues, we can disable checks on such cases.
> Only problem is that where previously —disable=check_free_strict did
> work, —disable=check_free does not seem to:)

The way I found to be the most impactful is to just keep a database of
the warnings and then:
1. mark the clearly invalid ones so they don't pop up again
2. every time there's a new patch - see if anything new gets added.

the new additions are the primary focus of new triage then since all
the old ones were presumably checked already.

if you make 2 execute before the patch is merged at the time of the
review, you also get ability to add these warnings into the patch
review process (I post to gerrit which is where we review our patches)
and then reviewers could pay more attention in those areas (hopefully)
just as the initial premise of static code analyzers was.

Re: apparent bug about check_free_strict

[Dan Carpenter]

On Tue, Nov 25, 2025 at 05:04:44PM +0200, Toomas Soome wrote:
> Only problem is that where previously —disable=check_free_strict did work,
> —disable=check_free does not seem to:)

Ugh...  I had CK(check_free) listed twice in check_list.h.  Try again.

regards,
dan carpenter

Re: apparent bug about check_free_strict

[Dan Carpenter]

On Tue, Nov 25, 2025 at 10:34:49AM -0500, Oleg Drokin wrote:
> On Tue, 2025-11-25 at 17:04 +0200, Toomas Soome wrote:
> > For positive side, the current smatch has been able to detect many
> > bugs the previous versions had missed - its definitely good progress.
> > While there are some issues, we can disable checks on such cases.
> > Only problem is that where previously —disable=check_free_strict did
> > work, —disable=check_free does not seem to:)
> 
> The way I found to be the most impactful is to just keep a database of
> the warnings and then:
> 1. mark the clearly invalid ones so they don't pop up again
> 2. every time there's a new patch - see if anything new gets added.
> 
> the new additions are the primary focus of new triage then since all
> the old ones were presumably checked already.

Yeah.  This is my favorite approach too.

I feel like you're more in favour of a higher false positive ratio
than I am, but I do think looking at false positives one time only
and then ignoring it forever is the best approach.  It lets you check
for more things, faster.

regards,
dan carpenter

Re: apparent bug about check_free_strict

[Dan Carpenter]

On Wed, Nov 26, 2025 at 04:47:04PM +0200, Toomas Soome wrote:
> 
> 
> > On 25. Nov 2025, at 16:50, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > 
> > On Tue, Nov 25, 2025 at 04:28:03PM +0200, Toomas Soome wrote:
> >> And another interesting case:
> >> 
> >> smatch is complaining about about ‘pptr’ but we do free ‘ptr’. 
> >> 
> >> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: adm_kef_util.c:1243 filter_mechlist() error: dereferencing freed memory 'pptr' (line 1242)
> >> 
> >> 1225 filter_mechlist(mechlist_t **pmechlist, const char *mech)
> >> 1226 {
> >> 1227         int             cnt = 0;
> >> 1228         mechlist_t      *ptr, *pptr;
> >> 1229         boolean_t       mech_present = B_FALSE;
> >> 1230  
> >> 1231         ptr = pptr = *pmechlist;
> >> 1232  
> >> 1233         while (ptr != NULL) {
> >> 1234                 if (strncmp(ptr->name, mech, sizeof (mech_name_t)) == 0) {
> >> 1235                         mech_present = B_TRUE;
> >> 1236                         if (ptr == *pmechlist) {
> >> 1237                                 pptr = *pmechlist = ptr->next;
> >> 1238                                 free(ptr);
> >> 1239                                 ptr = pptr;
> >> 1240                         } else {
> >> 1241                                 pptr->next = ptr->next;
> >> 1242                                 free(ptr);
> >> 1243                                 ptr = pptr->next;
> > 
> > This one is explainable...  Smatch is crap at loops, and only parses the
> > loop one time.  It might look like Smatch parses loops but it's all
> > hacks and special cases.
> > 
> > So, in this case, instead of seeing that "this is the second iteration
> > through the loop", Smatch says "this is dead code, but all of our other
> > assumptions are probably correct including that "ptr = pptr = *pmechlist".
> > 
> > So when we free "ptr" we're also freeing "pptr".
> > 
> > I've known the correct way to handle loops for over ten years now and
> > I partially wrote the code ten years ago.  But I've never wanted to do
> > it because it will slow everything down a lot.  It's quite a bit of
> > work as well, but mostly it was the slow down that was the issue.
> > But I think I'm going to try to make Smatch work better on other
> > projects outside the kernel so adding more and more loop hacks will
> > become less feasible and I will care less about slow downs so I
> > have decided I am going to do this work soon.
> > 
> > Basically you just parse every function twice and you store the next
> > iteration states for every loop.  Then you parse the functions again
> > and merge in the next iteration states.  It's a 2x slow down in
> > parsing.  I already have the --two-passes option but I haven't looked
> > at the output in a while...
> > 
> > regards,
> > dan carpenter
> 
> example about —two-passes:
> 
> tsoome@balrog:/code/illumos-gate/usr/src/cmd/cmd-inet/usr.lib/in.mpathd$ timex /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -O -m32 -Wall -Wextra -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unused-parameter -Wno-missing-field-initializers -Wno-array-bounds -p=illumos_user --disable=uninitialized,check_check_deref -Wno-vla -Wno-one-bit-signed-bitfield -Wno-external-function-has-definition -Wno-old-style-definition -Wno-strict-prototypes --fatal-checks --timeout=0 -Wno-maybe-uninitialized -std=gnu99 -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -fno-reorder-blocks-and-partition -fno-aggressive-loop-optimizations --param=max-inline-insns-single=450 -fstack-protector-strong -g -gdwarf-4 -gstrict-dwarf -std=gnu99 -DTEXT_DOMAIN="SUNW_OST_OSCMD" -D_TS_ERRNO -I/code/illumos-gate/proto/root_i386/usr/include -D_XOPEN_SOURCE=600 -D__EXTENSIONS__ -c mpd_main.c -o /tmp/cw.f6aqiX/cwi6aOiX.o 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: mpd_main.c:154 poll_add() warn: potentially one past the end of array 'newfds[i]'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: mpd_main.c:154 poll_add() warn: potentially one past the end of array 'newfds[i]'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: mpd_main.c:155 poll_add() warn: potentially one past the end of array 'newfds[i]'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: mpd_main.c:155 poll_add() warn: potentially one past the end of array 'newfds[i]'
> 
> real           1.16
> user           0.99
> sys            0.15
> 
> tsoome@balrog:/code/illumos-gate/usr/src/cmd/cmd-inet/usr.lib/in.mpathd$ timex /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch --two-passes -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -O -m32 -Wall -Wextra -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unused-parameter -Wno-missing-field-initializers -Wno-array-bounds -p=illumos_user --disable=uninitialized,check_check_deref -Wno-vla -Wno-one-bit-signed-bitfield -Wno-external-function-has-definition -Wno-old-style-definition -Wno-strict-prototypes --fatal-checks --timeout=0 -Wno-maybe-uninitialized -std=gnu99 -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -fno-reorder-blocks-and-partition -fno-aggressive-loop-optimizations --param=max-inline-insns-single=450 -fstack-protector-strong -g -gdwarf-4 -gstrict-dwarf -std=gnu99 -DTEXT_DOMAIN="SUNW_OST_OSCMD" -D_TS_ERRNO -I/code/illumos-gate/proto/root_i386/usr/include -D_XOPEN_SOURCE=600 -D__EXTENSIONS__ -c mpd_main.c -o /tmp/cw.3UaygX/cw5UaWgX.o  
> 
> real          19.70
> user          18.86
> sys            0.79
> 
> tsoome@balrog:/code/illumos-gate/usr/src/cmd/cmd-inet/usr.lib/in.mpathd$
> 
> Yes, it took longer, but also no complaints:)

Ugh...  19 times longer.  :(

I'm super not excited about that.  TBH, I haven't tested this in years...

regards,
dan carpenter