[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat approved this pull request.

LGTM, feel free to merge this. As @steakhal said, ensure that the PR 
title/description and the commit message all reflect the actual changes that 
you're commiting.

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


steakhal wrote:

Make sure you adjust/sync the commit title, content and the PR title before 
merging.

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/92424

From 769523d392204eac6c48cb80a2282212f3edbbe4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Fri, 10 May 2024 17:30:23 +0200
Subject: [PATCH 1/3] [clang][analyzer] Move checker
 alpha.security.cert.pos.34c into security.PutenvWithAuto .

The "cert" package looks not useful and the checker has not a meaningful name
with the old naming scheme.
Additionally tests and documentation is updated.
The checker looks good enough to be moved into non-alpha package.
---
 clang/docs/analyzer/checkers.rst  | 97 +--
 .../clang/StaticAnalyzer/Checkers/Checkers.td | 10 +-
 .../Analysis/cert/pos34-c-fp-suppression.cpp  | 51 --
 clang/test/Analysis/cert/pos34-c.cpp  | 61 
 clang/test/Analysis/putenv-with-auto.c| 66 +
 5 files changed, 119 insertions(+), 166 deletions(-)
 delete mode 100644 clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
 delete mode 100644 clang/test/Analysis/cert/pos34-c.cpp
 create mode 100644 clang/test/Analysis/putenv-with-auto.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index eb8b58323da4d..6ea768d003378 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
+Function ``putenv`` does not copy the passed string, only a pointer to the 
data is stored.
+Content of an automatic variable is likely to be overwritten after returning 
from the parent function.
+
+The problem can be solved by using a static variable or dynamically allocated
+memory. Even better is to avoid using ``putenv`` (it has other problems
+related to memory leaks) and use ``setenv`` instead.
+
+The check corresponds to CERT rule
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the 
argument
+`_.
+
+.. code-block:: c
+
+  int f() {
+char[] env = "NAME=value";
+return putenv(env); // putenv function should not be called with auto 
variables
+  }
+
+Limitations:
+
+   - In specific cases one can pass automatic variables to ``putenv``,
+ but one needs to ensure that the given environment key stays
+ alive until it's removed or overwritten.
+ Since the analyzer cannot keep track if and when the string passed to 
``putenv``
+ gets deallocated or overwritten, it needs to be slightly more aggressive
+ and warn for each case, leading in some cases to false-positive reports 
like this:
+
+ .. code-block:: c
+
+void baz() {
+  char env[] = "NAME=value";
+  putenv(env); // false-positive warning: putenv function should not 
be called...
+  // More code...
+  // FIXME: It looks like that if one string was passed to putenv,
+  // it should not be deallocated at all, because when reading the
+  // environment variable a pointer into this string is returned.
+  // In this case, if another (or the same) thread reads variable 
"NAME"
+  // at this point and does not copy the returned string, the data may
+  // become invalid.
+  putenv((char *)"NAME=anothervalue");
+  // This putenv call overwrites the previous entry, thus that can no 
longer dangle.
+} // 'env' array becomes dead only here.
+
 .. _unix-checkers:
 
 unix
@@ -2818,55 +2866,6 @@ alpha.security.cert
 
 SEI CERT checkers which tries to find errors based on their `C coding rules 
`_.
 
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^
-
-SEI CERT checkers of `POSIX C coding rules 
`_.
-
-.. _alpha-security-cert-pos-34c:
-
-alpha.security.cert.pos.34c
-"""
-Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
-
-.. code-block:: c
-
-  int func(const char *var) {
-char env[1024];
-int retval = snprintf(env, sizeof(env),"TEST=%s", var);
-if (retval < 0 || (size_t)retval >= sizeof(env)) {
-/* Handle error */
-}
-
-return putenv(env); // putenv function should not be called with auto 
variables
-  }
-
-Limitations:
-
-   - Technically, one can pass automatic variables to ``putenv``,
- but one needs to ensure that the given environment key stays
- alive until it's removed or overwritten.
- Since the analyzer cannot keep track of which envvars get overwritten
- 

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal approved this pull request.


https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -2792,6 +2792,31 @@ Warn on mmap() calls that are both writable and 
executable.
//   code
  }
 
+.. _alpha-security-putenv-stack-array:
+
+alpha.security.PutenvStackArray
+"""

steakhal wrote:

```suggestion
alpha.security.PutenvStackArray (C)
"""
```

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -2792,6 +2792,31 @@ Warn on mmap() calls that are both writable and 
executable.
//   code
  }
 
+.. _alpha-security-putenv-stack-array:
+
+alpha.security.PutenvStackArray
+"""
+Finds calls to the ``putenv`` function which pass a pointer to a 
stack-allocated
+(automatic) array as the argument. Function ``putenv`` does not copy the passed
+string, only a pointer to the data is stored and this data can be read even by
+other threads. Content of a stack-allocated array is likely to be overwritten
+after returning from the parent function.
+
+The problem can be solved by using a static array variable or dynamically
+allocated memory. Even better is to avoid using ``putenv`` (it has other
+problems related to memory leaks) and use ``setenv`` instead.
+
+The check corresponds to CERT rule
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the 
argument
+`_.
+
+.. code-block:: c
+
+  int f() {
+char[] env = "NAME=value";

steakhal wrote:

```suggestion
char env[] = "NAME=value";
```

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balázs Kéri via cfe-commits]

balazske wrote:

I moved the checker to `alpha.security` now and changed the name, and made the 
documentations more exact.

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/92424

From 769523d392204eac6c48cb80a2282212f3edbbe4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Fri, 10 May 2024 17:30:23 +0200
Subject: [PATCH 1/2] [clang][analyzer] Move checker
 alpha.security.cert.pos.34c into security.PutenvWithAuto .

The "cert" package looks not useful and the checker has not a meaningful name
with the old naming scheme.
Additionally tests and documentation is updated.
The checker looks good enough to be moved into non-alpha package.
---
 clang/docs/analyzer/checkers.rst  | 97 +--
 .../clang/StaticAnalyzer/Checkers/Checkers.td | 10 +-
 .../Analysis/cert/pos34-c-fp-suppression.cpp  | 51 --
 clang/test/Analysis/cert/pos34-c.cpp  | 61 
 clang/test/Analysis/putenv-with-auto.c| 66 +
 5 files changed, 119 insertions(+), 166 deletions(-)
 delete mode 100644 clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
 delete mode 100644 clang/test/Analysis/cert/pos34-c.cpp
 create mode 100644 clang/test/Analysis/putenv-with-auto.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index eb8b58323da4d..6ea768d003378 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
+Function ``putenv`` does not copy the passed string, only a pointer to the 
data is stored.
+Content of an automatic variable is likely to be overwritten after returning 
from the parent function.
+
+The problem can be solved by using a static variable or dynamically allocated
+memory. Even better is to avoid using ``putenv`` (it has other problems
+related to memory leaks) and use ``setenv`` instead.
+
+The check corresponds to CERT rule
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the 
argument
+`_.
+
+.. code-block:: c
+
+  int f() {
+char[] env = "NAME=value";
+return putenv(env); // putenv function should not be called with auto 
variables
+  }
+
+Limitations:
+
+   - In specific cases one can pass automatic variables to ``putenv``,
+ but one needs to ensure that the given environment key stays
+ alive until it's removed or overwritten.
+ Since the analyzer cannot keep track if and when the string passed to 
``putenv``
+ gets deallocated or overwritten, it needs to be slightly more aggressive
+ and warn for each case, leading in some cases to false-positive reports 
like this:
+
+ .. code-block:: c
+
+void baz() {
+  char env[] = "NAME=value";
+  putenv(env); // false-positive warning: putenv function should not 
be called...
+  // More code...
+  // FIXME: It looks like that if one string was passed to putenv,
+  // it should not be deallocated at all, because when reading the
+  // environment variable a pointer into this string is returned.
+  // In this case, if another (or the same) thread reads variable 
"NAME"
+  // at this point and does not copy the returned string, the data may
+  // become invalid.
+  putenv((char *)"NAME=anothervalue");
+  // This putenv call overwrites the previous entry, thus that can no 
longer dangle.
+} // 'env' array becomes dead only here.
+
 .. _unix-checkers:
 
 unix
@@ -2818,55 +2866,6 @@ alpha.security.cert
 
 SEI CERT checkers which tries to find errors based on their `C coding rules 
`_.
 
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^
-
-SEI CERT checkers of `POSIX C coding rules 
`_.
-
-.. _alpha-security-cert-pos-34c:
-
-alpha.security.cert.pos.34c
-"""
-Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
-
-.. code-block:: c
-
-  int func(const char *var) {
-char env[1024];
-int retval = snprintf(env, sizeof(env),"TEST=%s", var);
-if (retval < 0 || (size_t)retval >= sizeof(env)) {
-/* Handle error */
-}
-
-return putenv(env); // putenv function should not be called with auto 
variables
-  }
-
-Limitations:
-
-   - Technically, one can pass automatic variables to ``putenv``,
- but one needs to ensure that the given environment key stays
- alive until it's removed or overwritten.
- Since the analyzer cannot keep track of which envvars get overwritten
- 

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,66 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=security.PutenvWithAuto \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator.h"
+void free(void *);
+void *malloc(size_t);
+int putenv(char *);
+int snprintf(char *, size_t, const char *, ...);
+
+int test_auto_var(const char *var) {
+  char env[1024];
+  (void)snprintf(env, sizeof(env), "TEST=%s", var);
+  return putenv(env); // expected-warning{{The 'putenv' function should not be 
called with arguments that have automatic storage}}
+}
+
+int test_static_var(const char *var) {
+  static char env[1024];
+  (void)snprintf(env, sizeof(env), "TEST=%s", var);
+  return putenv(env);
+}
+
+void test_heap_memory(const char *var) {
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL)
+return;
+  if (putenv(env) != 0) // no-warning: env was dynamically allocated.
+free(env);
+}
+
+typedef struct {
+  int A;
+  char Env[1024];
+} Mem;
+
+int test_auto_var_struct() {
+  Mem mem;
+  return putenv(mem.Env); // expected-warning{{The 'putenv' function should 
not be called with arguments that have automatic storage}}
+}
+
+int test_auto_var_subarray() {
+  char env[1024];
+  return putenv(env + 100); // expected-warning{{The 'putenv' function should 
not be called with arguments that have automatic storage}}
+}
+
+int test_constant() {
+  char *env = "TEST";
+  return putenv(env);
+}
+
+extern char *ext_env;
+int test_extern() {
+  return putenv(ext_env); // no-warning: extern storage class.
+}
+
+void test_auto_var_reset() {
+  char env[] = "NAME=value";
+  // TODO: False Positive

NagyDonat wrote:

```suggestion
  // If a string was passed to putenv, it should not be deallocated at all,
  // because when reading the environment variable a pointer into this string
  // is returned.  In this case, if another (or the same) thread reads variable
  // "NAME" at this point and does not copy the returned string, the data may
  // become invalid.
```
As you explained in the `FIXME` added in the documentation, this is not a false 
positive. It's a good idea to keep this testcase, but let's replace the old 
`TODO` comment with your reasoning for reporting this.

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Donát Nagy via cfe-commits]

@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.

NagyDonat wrote:

```suggestion
Finds calls to the function ``putenv`` which pass a pointer to an automatic 
variable as the argument.
```

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Donát Nagy via cfe-commits]

@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
+Function ``putenv`` does not copy the passed string, only a pointer to the 
data is stored.
+Content of an automatic variable is likely to be overwritten after returning 
from the parent function.
+
+The problem can be solved by using a static variable or dynamically allocated
+memory. Even better is to avoid using ``putenv`` (it has other problems
+related to memory leaks) and use ``setenv`` instead.
+
+The check corresponds to CERT rule
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the 
argument
+`_.
+
+.. code-block:: c
+
+  int f() {
+char[] env = "NAME=value";
+return putenv(env); // putenv function should not be called with auto 
variables
+  }
+
+Limitations:
+
+   - In specific cases one can pass automatic variables to ``putenv``,
+ but one needs to ensure that the given environment key stays
+ alive until it's removed or overwritten.
+ Since the analyzer cannot keep track if and when the string passed to 
``putenv``
+ gets deallocated or overwritten, it needs to be slightly more aggressive
+ and warn for each case, leading in some cases to false-positive reports 
like this:
+
+ .. code-block:: c
+
+void baz() {
+  char env[] = "NAME=value";
+  putenv(env); // false-positive warning: putenv function should not 
be called...
+  // More code...
+  // FIXME: It looks like that if one string was passed to putenv,
+  // it should not be deallocated at all, because when reading the
+  // environment variable a pointer into this string is returned.
+  // In this case, if another (or the same) thread reads variable 
"NAME"
+  // at this point and does not copy the returned string, the data may
+  // become invalid.

NagyDonat wrote:

I agree with this remark.

I think you should just remove the whole `Limitations:` section and move this 
remark to the test file.

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat commented:

Thanks for bringing this checker out of alpha! I like the new name and I agree 
that the old `Limitations` section was incorrect; and I have some minor 
suggestions in inline comments.

I'd also ask for running this checker on some open source projects; but as 
`putenv` is an outdated function, I'd guess that there would be no results.

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Donát Nagy via cfe-commits]

@@ -1032,11 +1037,6 @@ let ParentPackage = ENV in {
 
 let ParentPackage = POSAlpha in {

NagyDonat wrote:

Please delete the packages that will no longer contain any checkers after this 
change. (As it's a bad naming scheme, they shouldn't be repopulated later.)

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
+Function ``putenv`` does not copy the passed string, only a pointer to the 
data is stored.
+Content of an automatic variable is likely to be overwritten after returning 
from the parent function.

steakhal wrote:

Even though it's formally called `automatic storage duration`, I'd say that 
`stack`-variable is more commonly understood among programmers.
Consequently, I'd suggest `security.PutenvWithStack` or 
`security.PutenvWithStackVar` instead. I think it would be easier to discover 
that way.
But I guess, this should be discussed separately.

https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

steakhal wrote:

> The "cert" package looks not useful and the checker has not a meaningful name 
> with the old naming scheme.
> Additionally tests and documentation is updated.
> The checker looks good enough to be moved into non-alpha package.

Personally, I prefer reviewing content changes separately from deciding/moving 
the checker out of alpha. Here are the reasons for doing so:
 - When content is moved around, a different set of mistakes can be made than 
when updating some doc content.
 - These are orthogonal decisions.

---

I'd suggest to split this PR into two:
 - Keep this PR for updating the content.
 - Have a separate PR for moving it out of alpha.



https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto (PR #92424)

[Balazs Benics via cfe-commits]

https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/92424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto . (PR #92424)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)


Changes

The "cert" package looks not useful and the checker has not a meaningful name 
with the old naming scheme.
Additionally tests and documentation is updated.
The checker looks good enough to be moved into non-alpha package.

---
Full diff: https://github.com/llvm/llvm-project/pull/92424.diff


5 Files Affected:

- (modified) clang/docs/analyzer/checkers.rst (+48-49) 
- (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+5-5) 
- (removed) clang/test/Analysis/cert/pos34-c-fp-suppression.cpp (-51) 
- (removed) clang/test/Analysis/cert/pos34-c.cpp (-61) 
- (added) clang/test/Analysis/putenv-with-auto.c (+66) 


``diff
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index eb8b58323da4d..6ea768d003378 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
+Function ``putenv`` does not copy the passed string, only a pointer to the 
data is stored.
+Content of an automatic variable is likely to be overwritten after returning 
from the parent function.
+
+The problem can be solved by using a static variable or dynamically allocated
+memory. Even better is to avoid using ``putenv`` (it has other problems
+related to memory leaks) and use ``setenv`` instead.
+
+The check corresponds to CERT rule
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the 
argument
+`_.
+
+.. code-block:: c
+
+  int f() {
+char[] env = "NAME=value";
+return putenv(env); // putenv function should not be called with auto 
variables
+  }
+
+Limitations:
+
+   - In specific cases one can pass automatic variables to ``putenv``,
+ but one needs to ensure that the given environment key stays
+ alive until it's removed or overwritten.
+ Since the analyzer cannot keep track if and when the string passed to 
``putenv``
+ gets deallocated or overwritten, it needs to be slightly more aggressive
+ and warn for each case, leading in some cases to false-positive reports 
like this:
+
+ .. code-block:: c
+
+void baz() {
+  char env[] = "NAME=value";
+  putenv(env); // false-positive warning: putenv function should not 
be called...
+  // More code...
+  // FIXME: It looks like that if one string was passed to putenv,
+  // it should not be deallocated at all, because when reading the
+  // environment variable a pointer into this string is returned.
+  // In this case, if another (or the same) thread reads variable 
"NAME"
+  // at this point and does not copy the returned string, the data may
+  // become invalid.
+  putenv((char *)"NAME=anothervalue");
+  // This putenv call overwrites the previous entry, thus that can no 
longer dangle.
+} // 'env' array becomes dead only here.
+
 .. _unix-checkers:
 
 unix
@@ -2818,55 +2866,6 @@ alpha.security.cert
 
 SEI CERT checkers which tries to find errors based on their `C coding rules 
`_.
 
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^
-
-SEI CERT checkers of `POSIX C coding rules 
`_.
-
-.. _alpha-security-cert-pos-34c:
-
-alpha.security.cert.pos.34c
-"""
-Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
-
-.. code-block:: c
-
-  int func(const char *var) {
-char env[1024];
-int retval = snprintf(env, sizeof(env),"TEST=%s", var);
-if (retval < 0 || (size_t)retval >= sizeof(env)) {
-/* Handle error */
-}
-
-return putenv(env); // putenv function should not be called with auto 
variables
-  }
-
-Limitations:
-
-   - Technically, one can pass automatic variables to ``putenv``,
- but one needs to ensure that the given environment key stays
- alive until it's removed or overwritten.
- Since the analyzer cannot keep track of which envvars get overwritten
- and when, it needs to be slightly more aggressive and warn for such
- cases too, leading in some cases to false-positive reports like this:
-
- .. code-block:: c
-
-void baz() {
-  char env[] = "NAME=value";
-  putenv(env); // false-positive warning: putenv function should not 
be called...
-  // More code...
-  putenv((char *)"NAME=anothervalue");
-   

[clang] [clang][analyzer] Move checker alpha.security.cert.pos.34c into security.PutenvWithAuto . (PR #92424)

[Balázs Kéri via cfe-commits]

https://github.com/balazske created 
https://github.com/llvm/llvm-project/pull/92424

The "cert" package looks not useful and the checker has not a meaningful name 
with the old naming scheme.
Additionally tests and documentation is updated.
The checker looks good enough to be moved into non-alpha package.

From 769523d392204eac6c48cb80a2282212f3edbbe4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Fri, 10 May 2024 17:30:23 +0200
Subject: [PATCH] [clang][analyzer] Move checker alpha.security.cert.pos.34c
 into security.PutenvWithAuto .

The "cert" package looks not useful and the checker has not a meaningful name
with the old naming scheme.
Additionally tests and documentation is updated.
The checker looks good enough to be moved into non-alpha package.
---
 clang/docs/analyzer/checkers.rst  | 97 +--
 .../clang/StaticAnalyzer/Checkers/Checkers.td | 10 +-
 .../Analysis/cert/pos34-c-fp-suppression.cpp  | 51 --
 clang/test/Analysis/cert/pos34-c.cpp  | 61 
 clang/test/Analysis/putenv-with-auto.c| 66 +
 5 files changed, 119 insertions(+), 166 deletions(-)
 delete mode 100644 clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
 delete mode 100644 clang/test/Analysis/cert/pos34-c.cpp
 create mode 100644 clang/test/Analysis/putenv-with-auto.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index eb8b58323da4d..6ea768d003378 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
+Function ``putenv`` does not copy the passed string, only a pointer to the 
data is stored.
+Content of an automatic variable is likely to be overwritten after returning 
from the parent function.
+
+The problem can be solved by using a static variable or dynamically allocated
+memory. Even better is to avoid using ``putenv`` (it has other problems
+related to memory leaks) and use ``setenv`` instead.
+
+The check corresponds to CERT rule
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the 
argument
+`_.
+
+.. code-block:: c
+
+  int f() {
+char[] env = "NAME=value";
+return putenv(env); // putenv function should not be called with auto 
variables
+  }
+
+Limitations:
+
+   - In specific cases one can pass automatic variables to ``putenv``,
+ but one needs to ensure that the given environment key stays
+ alive until it's removed or overwritten.
+ Since the analyzer cannot keep track if and when the string passed to 
``putenv``
+ gets deallocated or overwritten, it needs to be slightly more aggressive
+ and warn for each case, leading in some cases to false-positive reports 
like this:
+
+ .. code-block:: c
+
+void baz() {
+  char env[] = "NAME=value";
+  putenv(env); // false-positive warning: putenv function should not 
be called...
+  // More code...
+  // FIXME: It looks like that if one string was passed to putenv,
+  // it should not be deallocated at all, because when reading the
+  // environment variable a pointer into this string is returned.
+  // In this case, if another (or the same) thread reads variable 
"NAME"
+  // at this point and does not copy the returned string, the data may
+  // become invalid.
+  putenv((char *)"NAME=anothervalue");
+  // This putenv call overwrites the previous entry, thus that can no 
longer dangle.
+} // 'env' array becomes dead only here.
+
 .. _unix-checkers:
 
 unix
@@ -2818,55 +2866,6 @@ alpha.security.cert
 
 SEI CERT checkers which tries to find errors based on their `C coding rules 
`_.
 
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^
-
-SEI CERT checkers of `POSIX C coding rules 
`_.
-
-.. _alpha-security-cert-pos-34c:
-
-alpha.security.cert.pos.34c
-"""
-Finds calls to the ``putenv`` function which pass a pointer to an automatic 
variable as the argument.
-
-.. code-block:: c
-
-  int func(const char *var) {
-char env[1024];
-int retval = snprintf(env, sizeof(env),"TEST=%s", var);
-if (retval < 0 || (size_t)retval >= sizeof(env)) {
-/* Handle error */
-}
-
-return putenv(env); // putenv function should not be called with auto 
variables
-  }
-
-Limitations:
-
-   - Technically, one can pass automatic