[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp:1
+// RUN: %check_clang_tidy %s bugprone-fsetpos-argument-check %t
+

This is a C, not a C++ file, the extension should show it. In addition, a 
similar test should be added that uses `std::fgetpos()` and `std::fsetpos()`, 
and shows the matching still works.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

btw, if you're interested in exploring a static analyzer solution for this, 
here's a recent review of an analyzer feature that's in the same general 
problem space: https://reviews.llvm.org/D80018


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D79437#2052704 , @DerWaschbar wrote:

> In D79437#2052109 , @aaron.ballman 
> wrote:
>
> > Have you considered writing a static analyzer check so you can do data and 
> > control flow analysis to catch issues like these?
>
>
> I have noticed those issues too, but most likely the getter/setter will be in 
> the same function body and we could measure fast how common is that issue in 
> the wild.


That doesn't match my intuition, but if you have data, that would be excellent 
for helping to make a decision.

> Also, this was my first introductory project for Clang and with that, I can 
> rewrite this as a Static Analyzer project or start working on another 
> Clang-Tidy project.

Welcome! I think this functionality is likely useful in here as a clang-tidy 
check, but I'd be curious to see data on whether it finds true or false 
positives in the wild to help judge that. My gut instinct is that to do this 
properly, we'll want it in the static analyzer, but perhaps the tidy check is 
good enough. I'd be curious to know if others have different thoughts though 
(pinging @alexfh for visibility).


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Beka Grdzelishvili via Phabricator via cfe-commits]

DerWaschbar added a comment.

In D79437#2052109 , @aaron.ballman 
wrote:

> Thank you for working on this check, I think it's useful functionality. One 
> concern I have, though, is that it's not flow-sensitive and should probably 
> be implemented as a clang static analyzer check instead of a clang-tidy 
> check. For instance, consider these three plausible issues:
>
>   // This only sets the offset on one code path.
>   void func(FILE *fp) {
> fpos_t offset;
> if (condition) {
>   // ... code
>   if (0 != fgetpos(fp, ))
> return;
>   // ... code
> } else {
>  // ... code
> }
> fsetpos(fp, );
>   }
>  
>   // This doesn't check the failure from getting the position and sets the 
> position regardless.
>   void func(FILE *fp) {
> fpos_t offset;
> fgetpos(fp, );
> // ... code
> fsetpos(fp, );
>   }
>  
>   // This function accepts the offset from the caller but the caller passes 
> an invalid offset.
>   void func(FILE *fp, const fpos_t *offset) {
> fsetpos(fp, offset);
>   }
>   void caller(FILE *fp) {
> fpos_t offset;
> func(fp, );
>   }
>
>
> Have you considered writing a static analyzer check so you can do data and 
> control flow analysis to catch issues like these?


I have noticed those issues too, but most likely the getter/setter will be in 
the same function body and we could measure fast how common is that issue in 
the wild. Also, this was my first introductory project for Clang and with that, 
I can rewrite this as a Static Analyzer project or start working on another 
Clang-Tidy project.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Beka Grdzelishvili via Phabricator via cfe-commits]

DerWaschbar updated this revision to Diff 265930.
DerWaschbar marked 3 inline comments as done.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp
@@ -0,0 +1,59 @@
+// RUN: %check_clang_tidy %s bugprone-fsetpos-argument-check %t
+
+typedef unsigned int size_t;
+class FILE;
+struct fpos_t {};
+int fsetpos(FILE *stream, const fpos_t *pos);
+int fgetpos(FILE *stream, fpos_t *pos);
+void *memset(void *ptr, int value, size_t num);
+
+//-
+
+int opener1(FILE *file) {
+  int rc;
+  fpos_t offset;
+
+  if (file == nullptr ) {
+return -1;
+  }
+
+  rc = fgetpos(file, );
+  if (rc != 0 ) {
+return rc;
+  }
+
+  /* Read in data from file */
+
+  rc = fsetpos(file, );
+  if (rc != 0 ) {
+return rc;
+  }
+
+  return 0;
+}
+
+//-
+
+int opener2(FILE *file) {
+  int rc;
+  fpos_t offset;
+
+  memset(, 0, sizeof(offset));
+
+  if (file == nullptr) {
+return -1;
+  }
+
+  /* Read in data from file */
+
+  rc = fsetpos(file, );
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: file position indicator should be obtained only from 'fgetpos' [bugprone-fsetpos-argument-check]
+  if (rc != 0 ) {
+return rc;
+  }
+
+  return 0;
+}
+
+
+//-
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -57,6 +57,7 @@
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
`bugprone-forwarding-reference-overload `_,
+   `bugprone-fsetpos-argument-check ` _,
`bugprone-inaccurate-erase `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
@@ -315,6 +316,7 @@
`cert-err09-cpp `_, `misc-throw-by-value-catch-by-reference `_,
`cert-err61-cpp `_, `misc-throw-by-value-catch-by-reference `_,
`cert-fio38-c `_, `misc-non-copyable-objects `_,
+   `cert-fio44-c `_, `bugprone-fsetpos-argument-check `_,
`cert-msc30-c `_, `cert-msc50-cpp `_,
`cert-msc32-c `_, `cert-msc51-cpp `_,
`cert-oop11-cpp `_, `performance-move-constructor-init `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - cert-fio44-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-fsetpos-argument-check.html
+
+cert-fio44-c
+
+
+The cert-fio44-c check is an alias, please see
+`bugprone-fsetpos-argument-check `_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - bugprone-fsetpos-argument-check
+
+bugprone-fsetpos-argument-check
+===
+
+Finds call of ``fsetpos`` functions, which does not have file position indicator
+obtained from ``fgetpos``.
+
+.. code-block:: c
+
+#include 
+#include 
+
+int opener(FILE *file) {
+int rc;
+fpos_t offset;
+
+memset(, 0, sizeof(offset));
+
+if (file == NULL) {
+return -1;
+}
+
+/* Read in data from file */
+
+rc = fsetpos(file, );
+if (rc != 0 ) {
+return rc;
+}
+
+return 0;
+}
+
+
+This check corresponds to the CERT C Coding Standard rule
+`FIO44-C. Only use values for fsetpos() that are returned from fgetpos()
+`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- 

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this check, I think it's useful functionality. One 
concern I have, though, is that it's not flow-sensitive and should probably be 
implemented as a clang static analyzer check instead of a clang-tidy check. For 
instance, consider these three plausible issues:

  // This only sets the offset on one code path.
  void func(FILE *fp) {
fpos_t offset;
if (condition) {
  // ... code
  if (0 != fgetpos(fp, ))
return;
  // ... code
} else {
 // ... code
}
fsetpos(fp, );
  }
  
  // This doesn't check the failure from getting the position and sets the 
position regardless.
  void func(FILE *fp) {
fpos_t offset;
fgetpos(fp, );
// ... code
fsetpos(fp, );
  }
  
  // This function accepts the offset from the caller but the caller passes an 
invalid offset.
  void func(FILE *fp, const fpos_t *offset) {
fsetpos(fp, offset);
  }
  void caller(FILE *fp) {
fpos_t offset;
func(fp, );
  }

Have you considered writing a static analyzer check so you can do data and 
control flow analysis to catch issues like these?




Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:103
 // FIO
+
CheckFactories.registerCheck("cert-fio44-c");
 
CheckFactories.registerCheck("cert-fio38-c");

Can you reorder to below fio38-c?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst:4
+bugprone-fsetpos-argument-check
+
+

The underlines here don't look like they are correct.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst:35
+
+This check corresponds to the CERT C++ Coding Standard rule
+`FIO44-C. Only use values for fsetpos() that are returned from fgetpos()

the CERT C Coding Standard rule


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Beka Grdzelishvili via Phabricator via cfe-commits]

DerWaschbar updated this revision to Diff 264949.
DerWaschbar added a comment.

Rebased


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp
@@ -0,0 +1,59 @@
+// RUN: %check_clang_tidy %s bugprone-fsetpos-argument-check %t
+
+typedef unsigned int size_t;
+class FILE;
+struct fpos_t {};
+int fsetpos(FILE *stream, const fpos_t *pos);
+int fgetpos(FILE *stream, fpos_t *pos);
+void *memset(void *ptr, int value, size_t num);
+
+//-
+
+int opener1(FILE *file) {
+  int rc;
+  fpos_t offset;
+
+  if (file == nullptr ) {
+return -1;
+  }
+
+  rc = fgetpos(file, );
+  if (rc != 0 ) {
+return rc;
+  }
+
+  /* Read in data from file */
+
+  rc = fsetpos(file, );
+  if (rc != 0 ) {
+return rc;
+  }
+
+  return 0;
+}
+
+//-
+
+int opener2(FILE *file) {
+  int rc;
+  fpos_t offset;
+
+  memset(, 0, sizeof(offset));
+
+  if (file == nullptr) {
+return -1;
+  }
+
+  /* Read in data from file */
+
+  rc = fsetpos(file, );
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: file position indicator should be obtained only from 'fgetpos' [bugprone-fsetpos-argument-check]
+  if (rc != 0 ) {
+return rc;
+  }
+
+  return 0;
+}
+
+
+//-
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -57,6 +57,7 @@
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
`bugprone-forwarding-reference-overload `_,
+   `bugprone-fsetpos-argument-check ` _,
`bugprone-inaccurate-erase `_, "Yes"
`bugprone-incorrect-roundings `_,
`bugprone-infinite-loop `_,
@@ -315,6 +316,7 @@
`cert-err09-cpp `_, `misc-throw-by-value-catch-by-reference `_,
`cert-err61-cpp `_, `misc-throw-by-value-catch-by-reference `_,
`cert-fio38-c `_, `misc-non-copyable-objects `_,
+   `cert-fio44-c `_, `bugprone-fsetpos-argument-check `_,
`cert-msc30-c `_, `cert-msc50-cpp `_,
`cert-msc32-c `_, `cert-msc51-cpp `_,
`cert-oop11-cpp `_, `performance-move-constructor-init `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - cert-fio44-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-fsetpos-argument-check.html
+
+cert-fio44-c
+
+
+The cert-fio44-c check is an alias, please see
+`bugprone-fsetpos-argument-check `_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - bugprone-fsetpos-argument-check
+
+bugprone-fsetpos-argument-check
+
+
+Finds call of ``fsetpos`` functions, which does not have file position indicator
+obtained from ``fgetpos``.
+
+.. code-block:: c
+
+#include 
+#include 
+
+int opener(FILE *file) {
+int rc;
+fpos_t offset;
+
+memset(, 0, sizeof(offset));
+
+if (file == NULL) {
+return -1;
+}
+
+/* Read in data from file */
+
+rc = fsetpos(file, );
+if (rc != 0 ) {
+return rc;
+}
+
+return 0;
+}
+
+
+This check corresponds to the CERT C++ Coding Standard rule
+`FIO44-C. Only use values for fsetpos() that are returned from fgetpos()
+`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- 

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Beka Grdzelishvili via Phabricator via cfe-commits]

DerWaschbar updated this revision to Diff 262383.
DerWaschbar marked 7 inline comments as done.
DerWaschbar added a comment.

- Removed unnecessary empty lines.
- Added double back-ticks to highlight language constructs.
- Indented code in documentation.




Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-fsetpos-argument-check.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-fsetpos-argument-check.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-fsetpos-argument-check.cpp
@@ -0,0 +1,59 @@
+// RUN: %check_clang_tidy %s bugprone-fsetpos-argument-check %t
+
+typedef unsigned int size_t;
+class FILE;
+struct fpos_t {};
+int fsetpos(FILE *stream, const fpos_t *pos);
+int fgetpos(FILE *stream, fpos_t *pos);
+void *memset(void *ptr, int value, size_t num);
+
+//-
+  
+int opener1(FILE *file) {
+  int rc;
+  fpos_t offset;
+ 
+  if (file == nullptr ) {
+return -1;
+  }
+ 
+  rc = fgetpos(file, );
+  if (rc != 0 ) {
+return rc;
+  }
+ 
+  /* Read in data from file */
+ 
+  rc = fsetpos(file, );
+  if (rc != 0 ) {
+return rc;
+  }
+ 
+  return 0;
+}
+
+//-
+
+int opener2(FILE *file) {
+  int rc;
+  fpos_t offset;
+ 
+  memset(, 0, sizeof(offset));
+ 
+  if (file == nullptr) {
+return -1;
+  }
+ 
+  /* Read in data from file */
+ 
+  rc = fsetpos(file, );
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: file position indicator should be obtained only from 'fgetpos' [bugprone-fsetpos-argument-check]
+  if (rc != 0 ) {
+return rc;
+  }
+ 
+  return 0;
+}
+
+
+//-
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -47,6 +47,7 @@
bugprone-fold-init-type
bugprone-forward-declaration-namespace
bugprone-forwarding-reference-overload
+   bugprone-fsetpos-argument-check
bugprone-inaccurate-erase
bugprone-incorrect-roundings
bugprone-integer-division
@@ -95,6 +96,7 @@
cert-err60-cpp
cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) 
cert-fio38-c (redirects to misc-non-copyable-objects) 
+   cert-fio44-c
cert-flp30-c
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc32-c (redirects to cert-msc51-cpp) 
Index: clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - cert-fio44-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-fsetpos-argument-check.html
+
+cert-fio44-c
+
+
+The cert-fio44-c check is an alias, please see
+`bugprone-fsetpos-argument-check `_ 
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - bugprone-fsetpos-argument-check
+
+bugprone-fsetpos-argument-check
+
+
+Finds call of ``fsetpos`` functions, which does not have file position indicator 
+obtained from ``fgetpos``.
+
+.. code-block:: c
+
+#include 
+#include 
+
+int opener(FILE *file) {
+int rc;
+fpos_t offset;
+
+memset(, 0, sizeof(offset));
+
+if (file == NULL) {
+return -1;
+}
+
+/* Read in data from file */
+
+rc = fsetpos(file, );
+if (rc != 0 ) {
+return rc;
+}
+
+return 0;
+}
+
+
+This check corresponds to the CERT C++ Coding Standard rule
+`FIO44-C. Only use values for fsetpos() that are returned from fgetpos()
+`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Beka Grdzelishvili via Phabricator via cfe-commits]

DerWaschbar added a comment.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits