[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
aaron.ballman added a comment. In https://reviews.llvm.org/D29839#674714, @cryptoad wrote: > So if I understand correctly, the consensus is to abandon this and rewrite it > to be part of the clang static analyzer? That's correct. https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
cryptoad added a comment. So if I understand correctly, the consensus is to abandon this and rewrite it to be part of the clang static analyzer? https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. In https://reviews.llvm.org/D29839#674517, @aaron.ballman wrote: > In https://reviews.llvm.org/D29839#674301, @xazax.hun wrote: > > > Shouldn't this be a path sensitive check within the clang static analyzer > > instead? So branches are properly handled and interprocedural analysis is > > done. > > > I agree; I think this check should be part of the static analyzer because it > is path sensitive if we want it to be particularly useful. As it stands now, > it will catch trivial bugs, but by designing it as a clang-tidy check, it > isn't easily extensible to catch the bigger bugs across procedures. I totally agree with Aaron and Gabor. This analysis can't be properly implemented without path sensitivity and I can imagine many valid situations where it will be too noisy (custom functions or stream manipulators that hide width setting, for example). Clang-tidy has a bunch of lint-style analyses, but when there is a more appropriate technology to implement a certain analysis, it should be preferred. It's all trade-offs, but here path sensitive analysis seems to be a much better tool for the job. https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
aaron.ballman added a comment. In https://reviews.llvm.org/D29839#674301, @xazax.hun wrote: > Shouldn't this be a path sensitive check within the clang static analyzer > instead? So branches are properly handled and interprocedural analysis is > done. I agree; I think this check should be part of the static analyzer because it is path sensitive if we want it to be particularly useful. As it stands now, it will catch trivial bugs, but by designing it as a clang-tidy check, it isn't easily extensible to catch the bigger bugs across procedures. https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
Prazek added a comment. In https://reviews.llvm.org/D29839#674315, @xazax.hun wrote: > In https://reviews.llvm.org/D29839#674307, @Prazek wrote: > > > In https://reviews.llvm.org/D29839#674301, @xazax.hun wrote: > > > > > Shouldn't this be a path sensitive check within the clang static analyzer > > > instead? So branches are properly handled and interprocedural analysis is > > > done. > > > > > > Do you have some examples? I would argue, that even if you would have code > > that firstly uses width(), and then after a while reads input, then this is > > bugprone, and probably the line initializing width should be just before > > reading. > > > You are right, reasonable code sets the width right before reading the input. > But do we only want to catch bugs in reasonable code? We will catch bugs in resonable and not resonable code. But because clang-tidy is a linter, we will also warn about cases that might be valid, but looks buggy, making code resonable. We won't gonna have any false negatives (missed bugs), we only can have more false positives (warnings about correct code), but because it is linter, it totally make sense to warn about these cases. https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
xazax.hun added a comment. In https://reviews.llvm.org/D29839#674307, @Prazek wrote: > In https://reviews.llvm.org/D29839#674301, @xazax.hun wrote: > > > Shouldn't this be a path sensitive check within the clang static analyzer > > instead? So branches are properly handled and interprocedural analysis is > > done. > > > Do you have some examples? I would argue, that even if you would have code > that firstly uses width(), and then after a while reads input, then this is > bugprone, and probably the line initializing width should be just before > reading. You are right, reasonable code sets the width right before reading the input. But do we only want to catch bugs in reasonable code? https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
Prazek added a comment. In https://reviews.llvm.org/D29839#674301, @xazax.hun wrote: > Shouldn't this be a path sensitive check within the clang static analyzer > instead? So branches are properly handled and interprocedural analysis is > done. Do you have some examples? I would argue, that even if you would have code that firstly uses width(), and then after a while reads input, then this is bugprone, and probably the line initializing width should be just before reading. https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
xazax.hun added a comment. Shouldn't this be a path sensitive check within the clang static analyzer instead? So branches are properly handled and interprocedural analysis is done. https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
cryptoad updated this revision to Diff 88048.
cryptoad added a comment.
Missing line separators.
https://reviews.llvm.org/D29839
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/IstreamOverflowCheck.cpp
clang-tidy/misc/IstreamOverflowCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-istream-overflow.rst
test/clang-tidy/misc-istream-overflow.cpp
Index: test/clang-tidy/misc-istream-overflow.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-istream-overflow.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s misc-istream-overflow %t
+
+// Quick mock of std::istream and what we need to test our plug-in.
+namespace std {
+struct _Setw { int _M_n; };
+inline _Setw setw(int __n) { return { __n }; }
+typedef long int streamsize;
+template
+class basic_istream {
+ public:
+ typedef basic_istream<_CharT, _Traits> __istream_type;
+ basic_istream();
+ ~basic_istream();
+ bool eof() const { return false; }
+ streamsize width() const { return _M_width; }
+ streamsize width(streamsize __wide) {
+streamsize __old = _M_width;
+_M_width = __wide;
+return __old;
+ }
+ protected:
+ streamsize _M_width;
+};
+template struct char_traits {};
+template >
+ class basic_istream;
+typedef basic_istream istream;
+ template
+basic_istream<_CharT, _Traits>&
+operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s)
+{ return __in; }
+template
+ inline basic_istream<_CharT, _Traits>&
+ operator>>(basic_istream<_CharT, _Traits>& __is, _Setw __f)
+ {
+__is.width(__f._M_n);
+return __is;
+ }
+}
+
+void bad_1(std::istream ) {
+ char s[8];
+ is >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: istream::operator>> used without setting width
+}
+
+void bad_2(std::istream ) {
+ char s[8];
+ is.width(16);
+ is >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: width set for istream::operator>> (16) is greater than the destination buffer capacity (8)
+}
+
+void bad_3(std::istream ) {
+ char s[8];
+ is >> std::setw(16) >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: width set for istream::operator>> (16) is greater than the destination buffer capacity (8)
+}
+
+// The stream's width is reset to 0 at the end of operator>>, it is typically
+// not sufficient to set it prior to a loop, but it should be set within.
+void bad_3() {
+ std::istream is;
+ char s[8];
+ is.width(8);
+ while (!is.eof()) {
+is >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: istream::operator>> used without setting width
+ }
+}
+
+void good_1(std::istream ) {
+ char s[8];
+ is.width(8);
+ is >> s;
+}
+
+void good_2(std::istream ) {
+ char s[8];
+ is >> std::setw(8) >> s;
+}
+
+void good_3() {
+ std::istream is;
+ char s[8];
+ while (!is.eof()) {
+is.width(8);
+is >> s;
+ }
+}
Index: docs/clang-tidy/checks/misc-istream-overflow.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-istream-overflow.rst
@@ -0,0 +1,48 @@
+.. title:: clang-tidy - misc-istream-overflow
+
+misc-istream-overflow
+=
+
+This check finds calls to ``istream::operator>>`` (and derived classes) into a
+character buffer, that haven't set previously a width. This could result in a
+buffer overflow as the function will keep on reading until it reaches a space
+or EOF. If it finds an operation setting the width of the stream, the check
+will attempt to verify that the size fits the destination buffer.
+
+There are several ways to not have this problem surface:
+
+- call the ``width`` member function prior to using ``operator>>``, this will
+ ensure that only the given number of characters will be stored in the
+ destination buffer. Note that the width of the stream is reset to 0 after
+ each call to ``operator>>``, hence it must be set repeatedly if in a loop for
+ example.
+
+- use ``std::setw``, which in turns will set the width of the stream.
+
+- use an ``std::string`` as a destination instead of a character array, as this
+ will prevent any possibility of overflow.
+
+Given:
+
+.. code-block:: c++
+
+ std::istream is;
+ char s[8];
+
+It will trigger on:
+
+.. code-block:: c++
+
+ // The stream's width hasn't been set
+ is >> s;
+
+but not on:
+
+.. code-block:: c++
+
+ // The stream's width has been set through width()
+ is.width(8);
+ is >> s;
+
+ // The stream's width has been set through std::setw()
+ is >> std::setw(8) >> s;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -66,6 +66,7 @@
misc-inaccurate-erase
misc-incorrect-roundings
misc-inefficient-algorithm
+ misc-istream-overflow
misc-macro-parentheses
misc-macro-repeated-side-effects
misc-misplaced-const
Index: clang-tidy/misc/MiscTidyModule.cpp
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
cryptoad updated this revision to Diff 88047.
cryptoad added a comment.
Addressing first batch of comments.
https://reviews.llvm.org/D29839
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/IstreamOverflowCheck.cpp
clang-tidy/misc/IstreamOverflowCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-istream-overflow.rst
test/clang-tidy/misc-istream-overflow.cpp
Index: test/clang-tidy/misc-istream-overflow.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-istream-overflow.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s misc-istream-overflow %t
+
+// Quick mock of std::istream and what we need to test our plug-in.
+namespace std {
+struct _Setw { int _M_n; };
+inline _Setw setw(int __n) { return { __n }; }
+typedef long int streamsize;
+template
+class basic_istream {
+ public:
+ typedef basic_istream<_CharT, _Traits> __istream_type;
+ basic_istream();
+ ~basic_istream();
+ bool eof() const { return false; }
+ streamsize width() const { return _M_width; }
+ streamsize width(streamsize __wide) {
+streamsize __old = _M_width;
+_M_width = __wide;
+return __old;
+ }
+ protected:
+ streamsize _M_width;
+};
+template struct char_traits {};
+template >
+ class basic_istream;
+typedef basic_istream istream;
+ template
+basic_istream<_CharT, _Traits>&
+operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s)
+{ return __in; }
+template
+ inline basic_istream<_CharT, _Traits>&
+ operator>>(basic_istream<_CharT, _Traits>& __is, _Setw __f)
+ {
+__is.width(__f._M_n);
+return __is;
+ }
+}
+
+void bad_1(std::istream ) {
+ char s[8];
+ is >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: istream::operator>> used without setting width
+}
+
+void bad_2(std::istream ) {
+ char s[8];
+ is.width(16);
+ is >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: width set for istream::operator>> (16) is greater than the destination buffer capacity (8)
+}
+
+void bad_3(std::istream ) {
+ char s[8];
+ is >> std::setw(16) >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: width set for istream::operator>> (16) is greater than the destination buffer capacity (8)
+}
+
+// The stream's width is reset to 0 at the end of operator>>, it is typically
+// not sufficient to set it prior to a loop, but it should be set within.
+void bad_3() {
+ std::istream is;
+ char s[8];
+ is.width(8);
+ while (!is.eof()) {
+is >> s;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: istream::operator>> used without setting width
+ }
+}
+
+void good_1(std::istream ) {
+ char s[8];
+ is.width(8);
+ is >> s;
+}
+
+void good_2(std::istream ) {
+ char s[8];
+ is >> std::setw(8) >> s;
+}
+
+void good_3() {
+ std::istream is;
+ char s[8];
+ while (!is.eof()) {
+is.width(8);
+is >> s;
+ }
+}
Index: docs/clang-tidy/checks/misc-istream-overflow.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-istream-overflow.rst
@@ -0,0 +1,46 @@
+.. title:: clang-tidy - misc-istream-overflow
+
+misc-istream-overflow
+=
+
+This check finds calls to ``istream::operator>>`` (and derived classes) into a
+character buffer, that haven't set previously a width. This could result in a
+buffer overflow as the function will keep on reading until it reaches a space
+or EOF. If it finds an operation setting the width of the stream, the check
+will attempt to verify that the size fits the destination buffer.
+
+There are several ways to not have this problem surface:
+
+- call the ``width`` member function prior to using ``operator>>``, this will
+ ensure that only the given number of characters will be stored in the
+ destination buffer. Note that the width of the stream is reset to 0 after
+ each call to ``operator>>``, hence it must be set repeatedly if in a loop for
+ example.
+
+- use ``std::setw``, which in turns will set the width of the stream.
+
+- use an ``std::string`` as a destination instead of a character array, as this
+ will prevent any possibility of overflow.
+
+Given:
+
+.. code-block:: c++
+
+ std::istream is;
+ char s[8];
+
+It will trigger on:
+
+.. code-block:: c++
+ // The stream's width hasn't been set
+ is >> s;
+
+but not on:
+
+.. code-block:: c++
+ // The stream's width has been set through width()
+ is.width(8);
+ is >> s;
+
+ // The stream's width has been set through std::setw()
+ is >> std::setw(8) >> s;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -66,6 +66,7 @@
misc-inaccurate-erase
misc-incorrect-roundings
misc-inefficient-algorithm
+ misc-istream-overflow
misc-macro-parentheses
misc-macro-repeated-side-effects
misc-misplaced-const
Index: clang-tidy/misc/MiscTidyModule.cpp
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
cryptoad marked 5 inline comments as done.
cryptoad added inline comments.
Comment at: clang-tidy/misc/IstreamOverflowCheck.cpp:78-80
+if (!Arg->isIntegerConstantExpr(WidthSize, Context)) {
+ llvm::errs() << "Couldn't get width() size.\n";
+}
Prazek wrote:
> debug?
Oops!
I'd like to somehow keep that in there (for debug purposes). Any preferred way>
(ifdef DEBUG or other).
Repository:
rL LLVM
https://reviews.llvm.org/D29839
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
Prazek added a comment.
Nice check! :)
Comment at: clang-tidy/misc/IstreamOverflowCheck.cpp:59-61
+ if (ConstType) {
+ArraySize = ConstType->getSize();
+ }
same here
Comment at: clang-tidy/misc/IstreamOverflowCheck.cpp:78-80
+if (!Arg->isIntegerConstantExpr(WidthSize, Context)) {
+ llvm::errs() << "Couldn't get width() size.\n";
+}
debug?
Comment at: clang-tidy/misc/IstreamOverflowCheck.cpp:111-116
+if (HasWidthCall && WidthSize != 0) {
+ Width = WidthSize;
+}
+if (HasSetwCall && SetwSize != 0) {
+ Width = SetwSize;
+}
please remove unnecessary braces
Comment at: clang-tidy/misc/IstreamOverflowCheck.cpp:125-132
+ if (HasSetwCall) {
+diag(SetwCall->getLocation(), "std::setw called here",
+ DiagnosticIDs::Note);
+ }
+ if (HasWidthCall) {
+diag(WidthCall->getExprLoc(), "width called here",
+ DiagnosticIDs::Note);
same here
Repository:
rL LLVM
https://reviews.llvm.org/D29839
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
