Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Aaron Ballman via cfe-commits]

As a follow-up on this topic, I have proposed a patch in:
http://reviews.llvm.org/D15087

~Aaron

On Tue, Nov 3, 2015 at 1:05 PM, Aaron Ballman  wrote:
> On Tue, Nov 3, 2015 at 7:19 AM, Alexander Kornienko  wrote:
>> On Fri, Oct 9, 2015 at 12:13 PM, Aaron Ballman 
>> wrote:
>>>
>>> On Fri, Oct 9, 2015 at 3:09 PM, Daniel Berlin  wrote:
>>> > dberlin added a subscriber: dberlin.
>>> >
>>> > 
>>> > Comment at: docs/clang-tidy/checks/cert-variadic-function-def.rst:13
>>> > @@ +12,2 @@
>>> > +`DCL50-CPP. Do not define a C-style variadic function
>>> >
>>> > +`_.
>>> > 
>>> > I'm sure this is oversight on CERT's part, but their website actually
>>> > has terms of use (click the terms of use at the bottom of the page) that
>>> > says this can't be copied/reused, and here you are, copying it.
>>> > It explicit says: "
>>> > Use of the Service. You may only display the content of the Service for
>>> > your own personal use (i.e., non-commercial use) and may not otherwise 
>>> > copy,
>>> > reproduce, alter, modify, create derivative works, or publicly display any
>>> > content. "
>>> >
>>> > Before this is accepted, someone should email cert and say "hey, uh,
>>> > yeah, this seems bad", and get them to okay you doing this.
>>> > I'm sure they'll go and fix this.
>>>
>>> That's an excellent point, I will bring it up internally (I work for
>>> CERT) and report back.
>>
>>
>> Any news here?
>
> Yes; I have heard back from CERT's legal team, and they have a
> document that I have sent (off-list) to Daniel for review. If it seems
> like it would resolve his concerns, then I think the next step will be
> to bring it to the LLVM foundation more formally to see how they would
> like to handle it.
>
> ~Aaron
>
>>
>>>
>>>
>>> ~Aaron
>>>
>>> >
>>> >
>>> >
>>> > http://reviews.llvm.org/D13446
>>> >
>>> >
>>> >
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Daniel Berlin via cfe-commits]

Apologies, I will try to take a look today

On Tue, Nov 3, 2015, 10:05 AM Aaron Ballman  wrote:

> On Tue, Nov 3, 2015 at 7:19 AM, Alexander Kornienko 
> wrote:
> > On Fri, Oct 9, 2015 at 12:13 PM, Aaron Ballman 
> > wrote:
> >>
> >> On Fri, Oct 9, 2015 at 3:09 PM, Daniel Berlin 
> wrote:
> >> > dberlin added a subscriber: dberlin.
> >> >
> >> > 
> >> > Comment at: docs/clang-tidy/checks/cert-variadic-function-def.rst:13
> >> > @@ +12,2 @@
> >> > +`DCL50-CPP. Do not define a C-style variadic function
> >> >
> >> > +<
> https://www.securecoding.cert.org/confluence/display/cplusplus/DCL50-CPP.+Do+not+define+a+C-style+variadic+function
> >`_.
> >> > 
> >> > I'm sure this is oversight on CERT's part, but their website actually
> >> > has terms of use (click the terms of use at the bottom of the page)
> that
> >> > says this can't be copied/reused, and here you are, copying it.
> >> > It explicit says: "
> >> > Use of the Service. You may only display the content of the Service
> for
> >> > your own personal use (i.e., non-commercial use) and may not
> otherwise copy,
> >> > reproduce, alter, modify, create derivative works, or publicly
> display any
> >> > content. "
> >> >
> >> > Before this is accepted, someone should email cert and say "hey, uh,
> >> > yeah, this seems bad", and get them to okay you doing this.
> >> > I'm sure they'll go and fix this.
> >>
> >> That's an excellent point, I will bring it up internally (I work for
> >> CERT) and report back.
> >
> >
> > Any news here?
>
> Yes; I have heard back from CERT's legal team, and they have a
> document that I have sent (off-list) to Daniel for review. If it seems
> like it would resolve his concerns, then I think the next step will be
> to bring it to the LLVM foundation more formally to see how they would
> like to handle it.
>
> ~Aaron
>
> >
> >>
> >>
> >> ~Aaron
> >>
> >> >
> >> >
> >> >
> >> > http://reviews.llvm.org/D13446
> >> >
> >> >
> >> >
> >
> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Aaron Ballman via cfe-commits]

On Tue, Nov 3, 2015 at 7:19 AM, Alexander Kornienko  wrote:
> On Fri, Oct 9, 2015 at 12:13 PM, Aaron Ballman 
> wrote:
>>
>> On Fri, Oct 9, 2015 at 3:09 PM, Daniel Berlin  wrote:
>> > dberlin added a subscriber: dberlin.
>> >
>> > 
>> > Comment at: docs/clang-tidy/checks/cert-variadic-function-def.rst:13
>> > @@ +12,2 @@
>> > +`DCL50-CPP. Do not define a C-style variadic function
>> >
>> > +`_.
>> > 
>> > I'm sure this is oversight on CERT's part, but their website actually
>> > has terms of use (click the terms of use at the bottom of the page) that
>> > says this can't be copied/reused, and here you are, copying it.
>> > It explicit says: "
>> > Use of the Service. You may only display the content of the Service for
>> > your own personal use (i.e., non-commercial use) and may not otherwise 
>> > copy,
>> > reproduce, alter, modify, create derivative works, or publicly display any
>> > content. "
>> >
>> > Before this is accepted, someone should email cert and say "hey, uh,
>> > yeah, this seems bad", and get them to okay you doing this.
>> > I'm sure they'll go and fix this.
>>
>> That's an excellent point, I will bring it up internally (I work for
>> CERT) and report back.
>
>
> Any news here?

Yes; I have heard back from CERT's legal team, and they have a
document that I have sent (off-list) to Daniel for review. If it seems
like it would resolve his concerns, then I think the next step will be
to bring it to the LLVM foundation more formally to see how they would
like to handle it.

~Aaron

>
>>
>>
>> ~Aaron
>>
>> >
>> >
>> >
>> > http://reviews.llvm.org/D13446
>> >
>> >
>> >
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Daniel Berlin via cfe-commits]

dberlin added a subscriber: dberlin.


Comment at: docs/clang-tidy/checks/cert-variadic-function-def.rst:13
@@ +12,2 @@
+`DCL50-CPP. Do not define a C-style variadic function
+`_.

I'm sure this is oversight on CERT's part, but their website actually has terms 
of use (click the terms of use at the bottom of the page) that says this can't 
be copied/reused, and here you are, copying it.
It explicit says: "
Use of the Service. You may only display the content of the Service for your 
own personal use (i.e., non-commercial use) and may not otherwise copy, 
reproduce, alter, modify, create derivative works, or publicly display any 
content. "

Before this is accepted, someone should email cert and say "hey, uh, yeah, this 
seems bad", and get them to okay you doing this.
I'm sure they'll go and fix this.



http://reviews.llvm.org/D13446



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

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Aaron Ballman via cfe-commits]

On Fri, Oct 9, 2015 at 3:09 PM, Daniel Berlin  wrote:
> dberlin added a subscriber: dberlin.
>
> 
> Comment at: docs/clang-tidy/checks/cert-variadic-function-def.rst:13
> @@ +12,2 @@
> +`DCL50-CPP. Do not define a C-style variadic function
> +`_.
> 
> I'm sure this is oversight on CERT's part, but their website actually has 
> terms of use (click the terms of use at the bottom of the page) that says 
> this can't be copied/reused, and here you are, copying it.
> It explicit says: "
> Use of the Service. You may only display the content of the Service for your 
> own personal use (i.e., non-commercial use) and may not otherwise copy, 
> reproduce, alter, modify, create derivative works, or publicly display any 
> content. "
>
> Before this is accepted, someone should email cert and say "hey, uh, yeah, 
> this seems bad", and get them to okay you doing this.
> I'm sure they'll go and fix this.

That's an excellent point, I will bring it up internally (I work for
CERT) and report back.

~Aaron

>
>
>
> http://reviews.llvm.org/D13446
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Joerg Sonnenberger via cfe-commits]

On Mon, Oct 05, 2015 at 07:36:08PM +, Aaron Ballman via cfe-commits wrote:
> C-style variadic functions (using an ellipsis) can be dangerous in C++
> due to the inherit lack of type safety with argument passing. Better
> alternatives exist, such as function currying (like STL stream objects
> use), or function parameter packs. This patch adds a checker to
> diagnose definitions of variadic functions in C++ code, but still
> allows variadic function declarations, as those can be safely used to
> good effect for SFINAE patterns.

I would restrict this a bit to exclude function definitions with C
linkage. If you have such a ABI requirement, you normally can't replace
it with any of the alternatives.

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

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Joerg Sonnenberger via cfe-commits]

On Tue, Oct 06, 2015 at 04:20:05PM -0400, Aaron Ballman via cfe-commits wrote:
> On Tue, Oct 6, 2015 at 4:12 PM, Joerg Sonnenberger via cfe-commits
>  wrote:
> > On Mon, Oct 05, 2015 at 07:36:08PM +, Aaron Ballman via cfe-commits 
> > wrote:
> >> C-style variadic functions (using an ellipsis) can be dangerous in C++
> >> due to the inherit lack of type safety with argument passing. Better
> >> alternatives exist, such as function currying (like STL stream objects
> >> use), or function parameter packs. This patch adds a checker to
> >> diagnose definitions of variadic functions in C++ code, but still
> >> allows variadic function declarations, as those can be safely used to
> >> good effect for SFINAE patterns.
> >
> > I would restrict this a bit to exclude function definitions with C
> > linkage. If you have such a ABI requirement, you normally can't replace
> > it with any of the alternatives.
> 
> Under what circumstances would you run into this issue with C++ code?
> The only circumstance I can think of is if you have a C API that takes
> a function pointer to a varargs function as an argument. Is that the
> situation you are thinking of, or are there others?

Consider a stdio implementation written in C++? Wouldn't the
implementation of e.g. vprintf trigger exactly this warning?

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

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Aaron Ballman via cfe-commits]

On Tue, Oct 6, 2015 at 4:49 PM, Joerg Sonnenberger via cfe-commits
 wrote:
> On Tue, Oct 06, 2015 at 04:20:05PM -0400, Aaron Ballman via cfe-commits wrote:
>> On Tue, Oct 6, 2015 at 4:12 PM, Joerg Sonnenberger via cfe-commits
>>  wrote:
>> > On Mon, Oct 05, 2015 at 07:36:08PM +, Aaron Ballman via cfe-commits 
>> > wrote:
>> >> C-style variadic functions (using an ellipsis) can be dangerous in C++
>> >> due to the inherit lack of type safety with argument passing. Better
>> >> alternatives exist, such as function currying (like STL stream objects
>> >> use), or function parameter packs. This patch adds a checker to
>> >> diagnose definitions of variadic functions in C++ code, but still
>> >> allows variadic function declarations, as those can be safely used to
>> >> good effect for SFINAE patterns.
>> >
>> > I would restrict this a bit to exclude function definitions with C
>> > linkage. If you have such a ABI requirement, you normally can't replace
>> > it with any of the alternatives.
>>
>> Under what circumstances would you run into this issue with C++ code?
>> The only circumstance I can think of is if you have a C API that takes
>> a function pointer to a varargs function as an argument. Is that the
>> situation you are thinking of, or are there others?
>
> Consider a stdio implementation written in C++? Wouldn't the
> implementation of e.g. vprintf trigger exactly this warning?

It would, but it would also trigger a *lot* of warnings that aren't
directed at implementers as well, such as anything about relying on
implementation-defined or undefined behavior (like the implementation
of offsetof(), as a trivial example). It's a good point to keep in
mind though.

Thanks!

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

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Aaron Ballman via cfe-commits]

On Tue, Oct 6, 2015 at 4:12 PM, Joerg Sonnenberger via cfe-commits
 wrote:
> On Mon, Oct 05, 2015 at 07:36:08PM +, Aaron Ballman via cfe-commits wrote:
>> C-style variadic functions (using an ellipsis) can be dangerous in C++
>> due to the inherit lack of type safety with argument passing. Better
>> alternatives exist, such as function currying (like STL stream objects
>> use), or function parameter packs. This patch adds a checker to
>> diagnose definitions of variadic functions in C++ code, but still
>> allows variadic function declarations, as those can be safely used to
>> good effect for SFINAE patterns.
>
> I would restrict this a bit to exclude function definitions with C
> linkage. If you have such a ABI requirement, you normally can't replace
> it with any of the alternatives.

Under what circumstances would you run into this issue with C++ code?
The only circumstance I can think of is if you have a C API that takes
a function pointer to a varargs function as an argument. Is that the
situation you are thinking of, or are there others?

Thank you for the feedback!

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

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Samuel Benzaquen via cfe-commits]

sbenza accepted this revision.
sbenza added a comment.
This revision is now accepted and ready to land.

See comment regarding //CHECKs



Comment at: test/clang-tidy/cert-variadic-function-def.cpp:4
@@ +3,3 @@
+// Variadic function definitions are diagnosed.
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: do not define a C-style variadic 
function; consider using a function parameter pack or currying instead 
[cert-dcl50-cpp]
+void f1(int, ...) {}

All other tests I've read/written put the //CHECK after the matched lines.
We should be consistent with that.


http://reviews.llvm.org/D13446



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

[PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Aaron Ballman via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: alexfh, sbenza.
aaron.ballman added a subscriber: cfe-commits.

C-style variadic functions (using an ellipsis) can be dangerous in C++ due to 
the inherit lack of type safety with argument passing. Better alternatives 
exist, such as function currying (like STL stream objects use), or function 
parameter packs. This patch adds a checker to diagnose definitions of variadic 
functions in C++ code, but still allows variadic function declarations, as 
those can be safely used to good effect for SFINAE patterns.

This patch corresponds to the CERT C++ Coding Standard rule: 
https://www.securecoding.cert.org/confluence/display/cplusplus/DCL50-CPP.+Do+not+define+a+C-style+variadic+function

http://reviews.llvm.org/D13446

Files:
  clang-tidy/cert/CERTTidyModule.cpp
  clang-tidy/cert/CMakeLists.txt
  clang-tidy/cert/VariadicFunctionDefCheck.cpp
  clang-tidy/cert/VariadicFunctionDefCheck.h
  docs/clang-tidy/checks/cert-variadic-function-def.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cert-variadic-function-def.cpp

Index: test/clang-tidy/cert-variadic-function-def.cpp
===
--- test/clang-tidy/cert-variadic-function-def.cpp
+++ test/clang-tidy/cert-variadic-function-def.cpp
@@ -0,0 +1,18 @@
+// RUN: %python %S/check_clang_tidy.py %s cert-dcl50-cpp %t
+
+// Variadic function definitions are diagnosed.
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: do not define a C-style variadic function; consider using a function parameter pack or currying instead [cert-dcl50-cpp]
+void f1(int, ...) {}
+
+// Variadic function *declarations* are not diagnosed.
+void f2(int, ...); // ok
+
+// Function parameter packs are good, however.
+template 
+void f3(Arg F, Ts... Rest) {}
+
+struct S {
+  void f(int, ...); // ok
+  // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: do not define a C-style variadic function; consider using a function parameter pack or currying instead [cert-dcl50-cpp]
+  void f1(int, ...) {}
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -2,6 +2,7 @@
 =
 
 .. toctree::
+   cert-variadic-function-def
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/cert-variadic-function-def.rst
===
--- docs/clang-tidy/checks/cert-variadic-function-def.rst
+++ docs/clang-tidy/checks/cert-variadic-function-def.rst
@@ -0,0 +1,13 @@
+cert-dcl50-cpp
+
+
+A variadic function using an ellipsis has no mechanisms to check the type safety
+of arguments being passed to the function or to check that the number of
+arguments being passed matches the semantics of the function definition.
+Consequently, a runtime call to a C-style variadic function that passes
+inappropriate arguments yields undefined behavior. Such undefined behavior could
+be exploited to run arbitrary code.
+
+This check corresponds to the CERT C++ Coding Standard rule
+`DCL50-CPP. Do not define a C-style variadic function
+`_.
Index: clang-tidy/cert/VariadicFunctionDefCheck.h
===
--- clang-tidy/cert/VariadicFunctionDefCheck.h
+++ clang-tidy/cert/VariadicFunctionDefCheck.h
@@ -0,0 +1,34 @@
+//===--- VariadicFunctionDefCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_VARIADICFUNCTIONDEF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_VARIADICFUNCTIONDEF_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Guards against any C-style variadic function definitions (not declarations).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-variadic-function-def.html
+class VariadicFunctionDefCheck : public ClangTidyCheck {
+public:
+  VariadicFunctionDefCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_VARIADICFUNCTIONDEF_H
+
Index: clang-tidy/cert/VariadicFunctionDefCheck.cpp
===
--- 

Re: [PATCH] D13446: [PATCH] Add checker discouraging definition of variadic function definitions in C++

[Aaron Ballman via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Thank you for the note about checks -- I hadn't noticed that. I've fixed and 
committed in r249343.

Thank you for the quick review!

~Aaron


http://reviews.llvm.org/D13446



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