Re: apparent bug about check_free_strict

[Dan Carpenter <dan.carpenter@linaro.org>]

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