Skip to content

[PATCH] fix CHECK_this_length in sprintf.c

4 messages · Matthew Fowles Kulukundis, Martin Maechler

Original
Matthew Fowles Kulukundis
# 
All~

CHECK_this_length macro expands to multiple statements making it unsafe to
use in a single line `if` statement (as is happening near line 335).  This
fixes the macro using the standard `do { } while (0)` macro trick.

Matt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: r-sprintf.patch
Type: text/x-patch
Size: 758 bytes
Desc: not available
URL: <https://stat.ethz.ch/pipermail/r-devel/attachments/20160405/bbd3947a/attachment.bin>
1 day later
Martin Maechler
# 
> All~
    > CHECK_this_length macro expands to multiple statements making it unsafe to
    > use in a single line `if` statement (as is happening near line 335).  This
    > fixes the macro using the standard `do { } while (0)` macro trick.

yes, but you forgot the closing '}' ... so you could not even
have compiled R after applying your patch.  

Also, it would be nice to contrive a minimal example where the
change makes a difference.  This  "fails" to trigger :

--------------------------------
as.double.foo <- function(x) x[FALSE]
x <- structure(3, class="foo")
as.numeric(x) # numeric(0)

sprintf("%d !", x)# "3 !"  instead of giving an error
--------------------------------

Thank you, Matt, in any case; this (with the "{" !) has now gone
into the source.

Martin

    > Matt
    > x[DELETED ATTACHMENT external: r-sprintf.patch, text/x-patch]
    > ______________________________________________
    > R-devel at r-project.org mailing list
    > https://stat.ethz.ch/mailman/listinfo/r-devel
Matthew Fowles Kulukundis
# 
Martin~

Sorry about the bad patch.  I work on C++ at Google. We built a check for
clang-tidy that identifies errors of this form and discovered the error
here as part of our search. I am just trying to be a good citizen and
upstream a fix, but I must have gotten sloppy as I was doing a bunch of
these.

Thanks for fixing it and finding the a test for it, I actually had no idea
how to trigger this codepath and have never used R.

If you are curious, the upstream check for clang-tidy is
http://reviews.llvm.org/D18766
You may consider running some of the other clang-tidy checks on your source
base, they will likely find other bugs.

Cheers,
Matt

On Thu, Apr 7, 2016 at 6:46 AM, Martin Maechler <maechler at stat.math.ethz.ch>
wrote:

  
  


  


      

    

    

      
      

        


  

        

      Martin Maechler
    

    

      
      #
    

  


  

          

      
      
      
    

        
> Martin~
    > Sorry about the bad patch.  I work on C++ at Google. We built a check for
    > clang-tidy that identifies errors of this form and discovered the error
    > here as part of our search. I am just trying to be a good citizen and
    > upstream a fix, but I must have gotten sloppy as I was doing a bunch of
    > these.

Thank you, Matt, for the extra context... indeed the sloppyness was not a
big deal..

    > Thanks for fixing it and finding the a test for it, 
    > I actually had no idea how to trigger this codepath and have never used R.

I see.  But note that you misunderstood (probably my bad
English):

I did *NOT* find a test for it (and showed a case that did not
work as test, as it does not trigger an error [remember the
macro is CHECK_..() and actually shouled produce a useful error
message in a -- yet to find -- test case.


    > If you are curious, the upstream check for clang-tidy is
    > http://reviews.llvm.org/D18766
    > You may consider running some of the other clang-tidy checks on your source
    > base, they will likely find other bugs.

Thank you!

At the moment, I'm too overloaed with other duties / todos.
But it is good, to have this available, and other R-devel
readers may want to help the R core team by finding and patching
(or at least reporting) such bugs.

Martin

    > Cheers,
    > Matt

    > On Thu, Apr 7, 2016 at 6:46 AM, Martin Maechler <maechler at stat.math.ethz.ch>

        
> wrote:

        
>> >>>>> Matthew Fowles Kulukundis <matt.fowles at gmail.com>
    >> >>>>>     on Tue, 5 Apr 2016 11:17:30 -0400 writes:
    >> 
    >> > All~
    >> > CHECK_this_length macro expands to multiple statements making it
    >> unsafe to
    >> > use in a single line `if` statement (as is happening near line
    >> 335).  This
    >> > fixes the macro using the standard `do { } while (0)` macro trick.
    >> 
    >> yes, but you forgot the closing '}' ... so you could not even
    >> have compiled R after applying your patch.
    >> 
    >> Also, it would be nice to contrive a minimal example where the
    >> change makes a difference.  This  "fails" to trigger :
    >> 
    >> --------------------------------
    >> as.double.foo <- function(x) x[FALSE]
    >> x <- structure(3, class="foo")
    >> as.numeric(x) # numeric(0)
    >> 
    >> sprintf("%d !", x)# "3 !"  instead of giving an error
    >> --------------------------------
    >> 
    >> Thank you, Matt, in any case; this (with the "{" !) has now gone
    >> into the source.
    >> 
    >> Martin
    >> 
    >> > Matt
    >> > x[DELETED ATTACHMENT external: r-sprintf.patch, text/x-patch]
    >> > ______________________________________________
    >> > R-devel at r-project.org mailing list
    >> > https://stat.ethz.ch/mailman/listinfo/r-devel
    >> 

    > [[alternative HTML version deleted]]