Address of local auto-variable assigned to a function parameter

[Quipyowert2]

From: 38995150+Quipyowert2@users.noreply.github.com
Subject: Address of local auto-variable assigned to a function parameter
To: perl5-porters@perl.org
Cc: nathan@msi.localdomain
Reply-To: 38995150+Quipyowert2@users.noreply.github.com
Message-Id: 5.37.4_25310_1661728075@MSI

This is a bug report for perl from 38995150+Quipyowert2@users.noreply.github.com,
generated with the help of perlbug 1.42 running under perl 5.37.4.

---

[Please describe your issue here]
Perl 5.37.4 (305697f) has a mistake in pp_pack.c at line 942 in S_unpack_rec() and line 2252 in S_pack_rec() where the address of a stack allocated variable is assigned to a function parameter. When the function returns, the variable becomes invalid. I think perl should allocate some memory for a new tempsym_t (I don't know which macro to use for this) and then copy the contents of the lookahead or savesym to the newly allocated tempsym_t, like *symptr->previous = savesym; or *symptr->previous = lookahead;. At least I think that's how one copies a struct into to another struct in C.

I compiled Perl myself in Windows Subsystem for Linux to fuzz Perl with AFL++ but so far it hasn't found any crashing inputs yet. AFL++ did find a few hangs, but some of those hangs were because it used the sleep function in Perl, which doesn't count as a bug.

This bug was found with Cppcheck 2.9

Here are the errors from Cppcheck:

[//wsl$/openSUSE-Leap-15.4/home/nathan/src/perl5/pp_pack.c:942] (error) Dangerous assignment - the function parameter is assigned the address of a local auto-variable. Local auto-variables are reserved from the stack which is freed when the function ends. So the pointer to a local variable is invalid after the function ends. [autoVariable]
[//wsl$/openSUSE-Leap-15.4/home/nathan/src/perl5/pp_pack.c:2252] (error) Dangerous assignment - the function parameter is assigned the address of a local auto-variable. Local auto-variables are reserved from the stack which is freed when the function ends. So the pointer to a local variable is invalid after the function ends. [autoVariable]

My Windows version in case it is relevant: Windows 10 Home 21H2 OS Build (19044.1889).
Windows Subsystem for Linux version: openSUSE 15.4 running on WSL 1.

[Please do not change anything below this line]

---

Flags:
category=core
severity=low

Site configuration information for perl 5.37.4:

Configured by nathan at Fri Aug 26 18:52:42 PDT 2022.

Summary of my perl5 (revision 5 version 37 subversion 4) configuration:
Commit id: 305697f
Platform:
osname=linux
osvers=4.4.0-19041-microsoft
archname=x86_64-linux-thread-multi
uname='linux msi 4.4.0-19041-microsoft #1237-microsoft sat sep 11 14:32:00 pst 2021 x86_64 x86_64 x86_64 gnulinux '
config_args='-des -Dusedevel -Dusethreads -Dcc=afl-clang-lto'
hint=recommended
useposix=true
d_sigaction=define
useithreads=define
usemultiplicity=define
use64bitint=define
use64bitall=define
uselongdouble=undef
usemymalloc=n
default_inc_excludes_dot=define
Compiler:
cc='afl-clang-lto'
ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
optimize='-O2'
cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
ccversion=''
gccversion='Clang 13.0.1'
gccosandvers=''
intsize=4
longsize=8
ptrsize=8
doublesize=8
byteorder=12345678
doublekind=3
d_longlong=define
longlongsize=8
d_longdbl=define
longdblsize=16
longdblkind=3
ivtype='long'
ivsize=8
nvtype='double'
nvsize=8
Off_t='off_t'
lseeksize=8
alignbytes=8
prototype=define
Linker and Libraries:
ld='afl-clang-lto'
ldflags =' -fstack-protector-strong -L/usr/local/lib'
libpth=/usr/lib64/clang/13.0.1/lib /usr/local/lib /usr/x86_64-suse-linux/lib /usr/lib /lib64 /usr/lib64 /lib /usr/local/lib64
libs=-lpthread -ldl -lm -lcrypt -lutil -lc
perllibs=-lpthread -ldl -lm -lcrypt -lutil -lc
libc=libc-2.31.so
so=so
useshrplib=false
libperl=libperl.a
gnulibc_version='2.31'
Dynamic Linking:
dlsrc=dl_dlopen.xs
dlext=so
d_dlsymun=undef
ccdlflags='-Wl,-E'
cccdlflags='-fPIC'
lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'

---

@inc for perl 5.37.4:
/usr/local/lib/perl5/site_perl/5.37.4/x86_64-linux-thread-multi
/usr/local/lib/perl5/site_perl/5.37.4
/usr/local/lib/perl5/5.37.4/x86_64-linux-thread-multi
/usr/local/lib/perl5/5.37.4

---

Environment for perl 5.37.4:
HOME=/mnt/d/Linux_home/nathan
LANG=en_US.UTF-8
LANGUAGE (unset)
LD_LIBRARY_PATH (unset)
LOGDIR (unset)
PATH=/mnt/d/Linux_home/nathan/.cargo/bin:/mnt/d/Linux_home/nathan/perl5/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/mnt/c/Program Files/WindowsApps/46932SUSE.openSUSELeap15.4_154.1.735.0_x64__022rs5jcyhyac:/mnt/c/Python310/Scripts/:/mnt/c/Python310/:/mnt/c/Program Files (x86)/Common Files/Oracle/Java/javapath:/mnt/d/Python38-32/:/mnt/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/iCLS/:/mnt/c/Program Files/Intel/Intel(R) Management Engine Components/iCLS/:/mnt/c/Windows/system32:/mnt/c/Windows:/mnt/c/Windows/System32/Wbem:/mnt/c/Windows/System32/WindowsPowerShell/v1.0/:/mnt/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/DAL:/mnt/c/Program Files/Intel/Intel(R) Management Engine Components/DAL:/mnt/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/IPT:/mnt/c/Program
Files/Intel/Intel(R) Management Engine Components/IPT:/mnt/c/Program Files (x86)/NVIDIA Corporation/PhysX/Common:/mnt/c/Program Files/Intel/WiFi/bin/:/mnt/c/Program Files/Common Files/Intel/WirelessCommon/:/mnt/c/WINDOWS/system32:/mnt/c/WINDOWS:/mnt/c/WINDOWS/System32/Wbem:/mnt/c/WINDOWS/System32/WindowsPowerShell/v1.0/:/mnt/c/WINDOWS/System32/OpenSSH/:/mnt/c/ProgramData/chocolatey/bin:/mnt/c/Program Files/NVIDIA Corporation/NVIDIA NvDLISR:/mnt/c/WINDOWS/system32:/mnt/c/WINDOWS:/mnt/c/WINDOWS/System32/Wbem:/mnt/c/WINDOWS/System32/WindowsPowerShell/v1.0/:/mnt/c/WINDOWS/System32/OpenSSH/:/mnt/c/Program Files/Microsoft SQL Server/110/Tools/Binn/:/mnt/c/Program Files (x86)/Microsoft SDKs/TypeScript/1.0/:/mnt/c/Program Files/Microsoft SQL Server/120/Tools/Binn/:/mnt/d/Dwimperl/perl/bin:/mnt/d/Dwimperl/perl/site/bin:/mnt/c/TDM-GCC-64/bin:/mnt/c/Program Files/Meson/:/mnt/c/Program Files
(x86)/Common Files/Acronis/SnapAPI/:/mnt/c/Program Files (x86)/Common Files/Acronis/VirtualFile/:/mnt/c/Program Files (x86)/Common Files/Acronis/VirtualFile64/:/mnt/c/Program Files (x86)/Common Files/Acronis/FileProtector/:/mnt/c/Program Files (x86)/Common Files/Acronis/FileProtector64/:/mnt/c/Program Files/dotnet/:/mnt/d/Strawberry/c/bin:/mnt/d/Strawberry/perl/site/bin:/mnt/d/Strawberry/perl/bin:/mnt/d/Epic Games/Epic Games-Kyle/airshipper/:/mnt/d/Program Files/Git/cmd:/mnt/d/Program Files (x86)/nodejs/:/mnt/c/Program Files/LLVM/bin:/mnt/c/Users/nathan/.cargo/bin:/mnt/d/Python38/Scripts/:/mnt/d/Python38/:/mnt/c/Users/nathan/AppData/Local/Microsoft/WindowsApps:/mnt/c/Program Files/Oracle/VirtualBox:/mnt/d/msys64/usr/bin:/mnt/d/Program Files/CMake/bin:/mnt/c/tools/neovim/Neovim/bin:/mnt/d/Dr. Memory/bin/:/mnt/c/Program Files/OpenSSL-Win64/bin:/mnt/c/Program
Files/Java/jre1.8.0_271/bin:/mnt/d/Program Files (x86)/GnuWin32/lib:/mnt/c/Users/nathan/AppData/Local/Packages/PythonSoftwareFoundation.Python.3.7_qbz5n2kfra8p0/LocalCache/local-packages/Python37/Scripts:/mnt/c/Users/nathan/AppData/Local/Programs/Microsoft VS Code/bin:/mnt/c/Users/nathan/AppData/Roaming/npm:/mnt/d/Linux_home/nathan/.local/bin:/mnt/d/Linux_home/nathan/bin:/usr/local/bin:/usr/bin:/bin:/usr/lib64:/mnt/d/Linux_home/nathan/DrMemory-Linux-2.2.18249-1/bin64:/mnt/d/Linux_home/nathan/eclipse/cpp-2020-06/eclipse
PERL_BADLANG (unset)
SHELL=/bin/bash

[jkeenan]

Was there a particular invocation of cppcheck that you used to encounter the problematic code in pp_pack.c (i.e., something above and beyond cppcheck pp_pack.c)?

Would you have encountered these warnings if, instead of compiling with afl-clang-lto, you simply compiled with clang or gcc or g++?

[Quipyowert2]

I used the cppcheckgui.exe to analyze the code with "style" and "info" warnings turned off, 6 analyzer threads, and every checkbox in General tab of settings turned off. Then selected Analyze->Directory, selected my local clone of the perl5 repository, and waited.

I don't think the compiler I select with the -Dcc option to ./Configure affects CppCheck, because the version of Cppcheck I'm using is the native Windows one and I invoked ./Configure from WSL. Although cppcheck does have an option to analyze with clang. I have clang installed in Windows, but I'm not sure if cppcheck is using it.

For some reason when I copied the message from CppCheckGUI to Notepad++, the backslashes are changed to forward slashes, so the actual path is \\wsl$\openSUSE-Leap-15.4\home\nathan\src\perl5\pp_pack.c. Originally I had perl5 repo on my D: drive, but I ran into issue #18323 so I copied the .git folder inside perl5 to a WSL case-sensitive folder so that the Makefile could be generated.

[Quipyowert2]

After looking at the code again, I think this warning is harmless because in both places where the warning is emitted, after the while loop, savsym or lookahead is copied to the memory which symptr points to, presumably changing the previous pointer so it doesn't point at a stack variable anymore. If this was actually a problem, perl would probably crash when packing or unpacking.

It's not a false positive since the address of the stack variable is assigned to the previous pointer of symptr which is one of the parameters to S_unpack_rec/S_pack_rec.

I tried to fix the warning yesterday or the day before, but ended up creating a memory leak.

Here's a patch which hides the problem (inline suppressions have to be enabled in Cppcheck for this patch to work):

diff --git a/pp_pack.c b/pp_pack.c
index 4241338d09..2f0c3f0ada 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -939,6 +939,7 @@ S_unpack_rec(pTHX_ tempsym_t* symptr, const char *s, const char *strbeg, const c
             const U32 group_modifiers = TYPE_MODIFIERS(datumtype & ~symptr->flags);
             symptr->flags |= group_modifiers;
             symptr->patend = savsym.grpend;
+            // cppcheck-suppress autoVariable
             symptr->previous = &savsym;
             symptr->level++;
             PUTBACK;
@@ -2249,6 +2250,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
             symptr->flags |= group_modifiers;
             symptr->patend = savsym.grpend;
             symptr->level++;
+            // cppcheck-suppress autoVariable
             symptr->previous = &lookahead;
             while (len--) {
                 U32 was_utf8;

[Tux]

Can we please stick to non-C99 comment style please?

[Quipyowert2]

How about this patch? I just tried it with CppCheckGui with inline suppressions turned on and CppCheck doesn't output the errors anymore. I was about to create a bug report for Cppcheck because I thought inline suppressions in C89 comments didn't work but it turned out I misspelled the error id; it's actually autoVariables not autoVariable.

diff --git a/pp_pack.c b/pp_pack.c
index 4241338d09..4442287ae4 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -939,6 +939,7 @@ S_unpack_rec(pTHX_ tempsym_t* symptr, const char *s, const char *strbeg, const c
             const U32 group_modifiers = TYPE_MODIFIERS(datumtype & ~symptr->flags);
             symptr->flags |= group_modifiers;
             symptr->patend = savsym.grpend;
+            /* cppcheck-suppress autoVariables */
             symptr->previous = &savsym;
             symptr->level++;
             PUTBACK;
@@ -2249,6 +2250,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
             symptr->flags |= group_modifiers;
             symptr->patend = savsym.grpend;
             symptr->level++;
+            /* cppcheck-suppress autoVariables */
             symptr->previous = &lookahead;
             while (len--) {
                 U32 was_utf8;

[tonycoz]

I think the suppression markup is the correct solution instead of the fix in #20310.

The code is linking an auto variable into the chain for the recursive call, but then unlinks it immediately after in each case, so I don't think we have a bug here.

The change in #20310 looks very fragile, adding over 40 calls (I stopped counting) to free a block which the normal stack unwinding in the existing code already deals with.

It isn't necessary here, but there is a way to safely free the memory in this type of circumstance, using code like:

   Newx(someptr, ...);
   SAVEFREEPV(ptr);
   recursive_call(ptr);

This by itself will work, but has problems in this case, since this case may be called many, many times, leaving all the buffers allocated, and using a great deal of space on the save stack until we see the enclosing LEAVE. To avoid that you can do:

   ENTER;
   Newx(someptr, ...);
   SAVEFREEPV(ptr);
   recursive_call(ptr);
   LEAVE; /* ptr freed here */

but that has a performance cost too.

[tonycoz]

I do think cppcheck, and clang-tidy, CodeQL, are useful tools, and I think it would be good to integrate them into our CI. They have a shorter turnaround compared to Coverity, and if they're part of our CI they'd handle branches better than we can with Coverity.

I do think, as with Coverity, need to treat each report critically, and not just blindly appease the tool.

I'd welcome as PR that integrated cppcheck into our CI and included patches to the problems it found, and with (hopefully few) suppressions for the non-problems it found.