[Bug libstdc++/50268] New: [C++0x] bitset doesn't sanitize input

[Bug libstdc++/50268] New: [C++0x] bitset doesn't sanitize input

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

             Bug #: 50268
           Summary: [C++0x] bitset doesn't sanitize input
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: marc.glisse@normalesup.org


#include <bitset>
#include <iostream>
int main(){
        std::bitset<1> b(3);
        std::cout << b.count() << std::endl;
}

Prints 1 with -std=c++98 and 2 with -std=c++0x.

(I noticed it while wondering why _M_are_all_aux uses popcount, which isn't
optimal)

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #1 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-01 19:23:37 UTC ---
Any idea where the problem is?

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #2 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-01 19:29:23 UTC ---
Urgh, I can't believe it's here:

#ifdef __GXX_EXPERIMENTAL_CXX0X__
      constexpr bitset(unsigned long long __val) noexcept
      : _Base(__val) { }
#else

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2011-09-01
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com
     Ever Confirmed|0                           |1

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #3 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-01 22:17:00 UTC ---
Created attachment 25173
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25173
Draft patch

Hi Marc. This is a patch I'm working on. Can you see anything obviously wrong
with it? And / or simple improvements? (we want the fix for 4.6 too, I think)

The issue of course is that for a constexpr constructor we can't do anything in
the body (at the time "somebody" decided to disregard the issue completely ;)
but luckily we can on the unsigned long long itself with a constexpr helper.
Overall I think we are not doing more work here compared to C++03.

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25173|0                           |1
        is obsolete|                            |

--- Comment #4 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-01 22:54:43 UTC ---
Comment on attachment 25173
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25173
Draft patch

S*it, patch is badly wrong: in general we really want to zero the unused bits
of the highest word not of the __val. But the highest word either is all zeros
anyway or has excess non zero bits beyond _Nb because written by __val, thus we
have to restrict the zeroing of the highest bits of __val to this case.

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 01:35:06 UTC ---
Created attachment 25174
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25174
Updated draft

This one is much better, and actually should lead to slightly better code than
C++98, because we don't do anything if _Nw > 1 (the 32-bit case is also better
but doesn't optimize the case _Nb % _GLIBCXX_BITSET_BITS_PER_WORD == 0 && _Nb %
_GLIBCXX_BITSET_BITS_PER_ULL != 0. I don't care much these times)

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #6 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-02 08:03:22 UTC ---
(In reply to comment #5)
> This one is much better, and actually should lead to slightly better code than
> C++98, because we don't do anything if _Nw > 1 (the 32-bit case is also better
> but doesn't optimize the case _Nb % _GLIBCXX_BITSET_BITS_PER_WORD == 0 && _Nb %
> _GLIBCXX_BITSET_BITS_PER_ULL != 0. I don't care much these times)

Looks better indeed. I think the compiler should be responsible for optimizing
x&~0UL, not the library. I'll have to check that bitset<32>(x).count() has no
overhead compared to a call to __builtin_popcount.

Looks to me like _DoWork is actually _Nb<_GLIBCXX_BITSET_BITS_PER_ULL (more
intuitive, and it makes _Nw and _Extrabits useless). I usually write the number
~((~static_cast<unsigned long long>(0)) << _Extrabits) as (1ULL <<
_Extrabits)-1 and just noticed that your version would be faster at runtime
(here it is compile-time anyway), cool.

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #7 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 09:37:55 UTC ---
Hi,

(In reply to comment #6)
> Looks better indeed. I think the compiler should be responsible for optimizing
> x&~0UL, not the library. I'll have to check that bitset<32>(x).count() has no
> overhead compared to a call to __builtin_popcount.

Indeed, I had the same thought about the compiler. And really, we are doing
anyway better than C++98 for 32-bit too, I'm not particularly worried. But, if
we have time we should check and open an optimization PR in case.

> Looks to me like _DoWork is actually _Nb<_GLIBCXX_BITSET_BITS_PER_ULL (more
> intuitive, and it makes _Nw and _Extrabits useless). I usually write the number
> ~((~static_cast<unsigned long long>(0)) << _Extrabits) as (1ULL <<
> _Extrabits)-1 and just noticed that your version would be faster at runtime
> (here it is compile-time anyway), cool.

Ah great. I'm so stupid, trying to do all the work in terms of _Nw and so on,
where in this case we have _Nb itself available. About the formula, interesting
indeed what you are noticing, I guess I will stick to the more obfuscated one
for compile-time too, because like this it's clear we are doing the same
adjustment done normally in _M_do_sanitize at run-time. I'm attaching the
updated patch I'm going to test and commit (4_6-branch too).

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25174|0                           |1
        is obsolete|                            |

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 09:40:42 UTC ---
Created attachment 25175
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25175
Updated (simplified) again per Marc

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #9 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2011-09-02 10:28:40 UTC ---
Author: paolo
Date: Fri Sep  2 10:28:36 2011
New Revision: 178463

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=178463
Log:
2011-09-02  Paolo Carlini  <paolo.carlini@oracle.com>
        Marc Glisse  <marc.glisse@normalesup.org>

    PR libstdc++/50268
    * include/std/bitset (struct _Sanitize_val): Add.
    (bitset<>::bitset(unsigned long long)): Fix.
    * testsuite/23_containers/bitset/cons/50268.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/23_containers/bitset/cons/50268.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/bitset

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.6.2

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #10 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 10:41:52 UTC ---
(by the way, if you can see a neat enough way to improve _M_are_all_aux, you
are welcome to propose it! I'm definitely not an expert in this area, and when
I implemented it, I remember just quickly hacking something close to what we
have elsewhere, which I could easily convince myself was correct)

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #11 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-02 10:59:46 UTC ---
(In reply to comment #10)
> (by the way, if you can see a neat enough way to improve _M_are_all_aux, you
> are welcome to propose it! I'm definitely not an expert in this area, and when
> I implemented it, I remember just quickly hacking something close to what we
> have elsewhere, which I could easily convince myself was correct)

all(), when it reaches the last chunk, checks
(nbchunk-1)*chunksize+popcount(lastchunk)==size(). It seems to me that it
should check instead lastchunk==mask (where mask is something like
~(~0UL<<extrabits), computed at compile-time). Because of the way things are
implemented, it means the helper needs to take mask (or extrabits) as argument.

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #12 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 12:08:04 UTC ---
Created attachment 25178
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25178
Work in progress patch for the _M_are_all_aux issue

I'm considering doing something like this: what do you think? Can we tidy
somehow the general case (_Nw > 1)? (patch already passes the existing
bitset::all test)

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #13 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-02 12:23:33 UTC ---
(In reply to comment #12)
> Created attachment 25178 [details]
> Work in progress patch for the _M_are_all_aux issue
> 
> I'm considering doing something like this: what do you think? Can we tidy
> somehow the general case (_Nw > 1)? (patch already passes the existing
> bitset::all test)

Doesn't _Base_bitset<1> also need a special case for
_Nb==_GLIBCXX_BITSET_BITS_PER_WORD ?
I think we could express the constant as
~static_cast<_WordT>(0)>>(_GLIBCXX_BITSET_BITS_PER_WORD-_Nb) so we don't need a
special case (and something similar for the last word of the generic
_Base_bitset)

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #14 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 12:33:40 UTC ---
(In reply to comment #13)
> Doesn't _Base_bitset<1> also need a special case for
> _Nb==_GLIBCXX_BITSET_BITS_PER_WORD ?

You are right, got confused by the signedness.

> I think we could express the constant as
> ~static_cast<_WordT>(0)>>(_GLIBCXX_BITSET_BITS_PER_WORD-_Nb) so we don't need a
> special case (and something similar for the last word of the generic
> _Base_bitset)

Let's see what I can do, thanks.

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25178|0                           |1
        is obsolete|                            |

--- Comment #15 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 13:15:38 UTC ---
Created attachment 25179
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25179
Updated per Marc draft for the _M_are_all_aux issue

I'm finishing testing this.

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #16 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-02 13:26:14 UTC ---
(In reply to comment #15)
> I'm finishing testing this.

Looks good, thanks.

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

--- Comment #17 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2011-09-02 13:39:26 UTC ---
Author: paolo
Date: Fri Sep  2 13:39:22 2011
New Revision: 178475

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=178475
Log:
2011-09-02  Paolo Carlini  <paolo.carlini@oracle.com>
        Marc Glisse  <marc.glisse@normalesup.org>

    PR libstdc++/50268
    * include/std/bitset (struct _Sanitize_val): Add.
    (bitset<>::bitset(unsigned long long)): Fix.
    * testsuite/23_containers/bitset/cons/50268.cc: New.

Added:
   
branches/gcc-4_6-branch/libstdc++-v3/testsuite/23_containers/bitset/cons/50268.cc
Modified:
    branches/gcc-4_6-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_6-branch/libstdc++-v3/include/std/bitset

[Bug libstdc++/50268] [C++0x] bitset doesn't sanitize input

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50268

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #18 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-02 13:40:40 UTC ---
Fixed for 4.6.2 and mainline (_M_are_all_aux issue dealt with in mainline).