You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Farid Zaripov <Fa...@epam.com> on 2007/05/24 13:18:02 UTC
[PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
> -----Original Message-----
> From: Martin Sebor (JIRA) [mailto:jira@apache.org]
> Sent: Thursday, May 24, 2007 1:14 AM
> To: stdcxx-dev@incubator.apache.org
> Subject: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
>
> SIGSEGV in istringstream::str()
> -------------------------------
>
> Key: STDCXX-427
> URL: https://issues.apache.org/jira/browse/STDCXX-427
> Project: C++ Standard Library
> Issue Type: Bug
> Components: 27. Input/Output
> Affects Versions: 4.2
> Environment: gcc-3.3.3 on Linux
> Reporter: Martin Sebor
> Priority: Critical
>
>
> This only happens on trunk, not with 4.1.3:
>
> $ rm t.o && cat t.cpp && make t && gdb -q t #include
> <cassert> #include <sstream> #include <string>
>
> int main ()
> {
> using namespace std;
>
> string str ("test");
>
> istringstream isstr;
>
> isstr.str (str);
>
> assert (isstr.str ().length () == str.length ());
>
> assert (isstr.str () == str);
> }
The bug also reproduced on MSVC. The proposed patch below:
-----------------------
Index: sstream
===================================================================
--- sstream (revision 541218)
+++ sstream (working copy)
@@ -126,7 +126,7 @@
_C_string_type str () const {
const _RWSTD_SIZE_T __slen = (this->egptr () < this->pptr () ?
- this->pptr () : this->egptr ()) - this->pbase ();
+ this->pptr () : this->egptr ()) - this->_C_buffer;
return _C_string_type (this->_C_buffer, __slen);
}
-----------------------
Farid.
RE: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
Posted by Farid Zaripov <Fa...@epam.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Tuesday, May 29, 2007 9:33 PM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV
> in istringstream::str()
>
> > Formally the test case is valid, but logically, I think,
> the buffer
> > pointers and the buffer itself should be in consistency,
> and in this
> > test eback() and pbase() are changed from the real begin of
> the buffer
> > of the std::stringbuf.
>
> I'm not sure I understand. If the test case is valid but
> doesn't pass, the implementation doesn't conform. The only
> way this could be acceptable is if the requirement in the
> standard was unclear or didn't make sense (in which case we'd
> need to open an issue against the standard to clarify/correct
> the requirement). To me the requirement in the standard is
> clear and makes sense. Does it not to you?
The requirement in the standard is clear to me too.
Farid.
Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
Posted by Martin Sebor <se...@roguewave.com>.
Farid Zaripov wrote:
>> -----Original Message-----
>> From: Martin Sebor [mailto:sebor@roguewave.com]
>> Sent: Saturday, May 26, 2007 3:05 AM
>> To: stdcxx-dev@incubator.apache.org
>> Subject: Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV
>> in istringstream::str()
>>
>>> The bug is in that len calculated as highmark - pbase(),
>> but pbase()
>>> == 0 when buffer opened only in input mode. The begin of
>> the buffer is
>>> always
>>> this->_C_buffer with any openmode. So len = highmark - _C_buffer is
>>> correct, I think.
>> In input mode (only) the function is supposed to return:
>>
>> string(eback(), egptr());
>>
>> In output mode (or input | output) the function must return:
>>
>> string(pbase(), high_mark);
>>
>> So unless I'm missing something the program below should be a
>> valid (albeit incomplete) test case. Let me know what you think.
>
> Formally the test case is valid, but logically, I think, the buffer
> pointers and the buffer
> itself should be in consistency, and in this test eback() and pbase()
> are changed from
> the real begin of the buffer of the std::stringbuf.
I'm not sure I understand. If the test case is valid but doesn't
pass, the implementation doesn't conform. The only way this could
be acceptable is if the requirement in the standard was unclear
or didn't make sense (in which case we'd need to open an issue
against the standard to clarify/correct the requirement). To me
the requirement in the standard is clear and makes sense. Does
it not to you?
Martin
RE: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
Posted by Farid Zaripov <Fa...@epam.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Saturday, May 26, 2007 3:05 AM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV
> in istringstream::str()
>
> > The bug is in that len calculated as highmark - pbase(),
> but pbase()
> > == 0 when buffer opened only in input mode. The begin of
> the buffer is
> > always
> > this->_C_buffer with any openmode. So len = highmark - _C_buffer is
> > correct, I think.
>
> In input mode (only) the function is supposed to return:
>
> string(eback(), egptr());
>
> In output mode (or input | output) the function must return:
>
> string(pbase(), high_mark);
>
> So unless I'm missing something the program below should be a
> valid (albeit incomplete) test case. Let me know what you think.
Formally the test case is valid, but logically, I think, the buffer
pointers and the buffer
itself should be in consistency, and in this test eback() and pbase()
are changed from
the real begin of the buffer of the std::stringbuf.
Farid.
RE: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
Posted by Farid Zaripov <Fa...@epam.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Saturday, May 26, 2007 3:05 AM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV
> in istringstream::str()
>
> In input mode (only) the function is supposed to return:
>
> string(eback(), egptr());
>
> In output mode (or input | output) the function must return:
>
> string(pbase(), high_mark);
>
> So unless I'm missing something the program below should be a
> valid (albeit incomplete) test case. Let me know what you think.
>
> #include <cassert>
> #include <cstdio>
> #include <sstream>
>
> int main ()
> {
> struct Buf: std::stringbuf {
> Buf (std::string s, std::ios::openmode m)
> : std::stringbuf (s, m) { }
>
> void setget (int beg, int cur, int end) {
> setg (eback () + beg, eback () + cur, eback () + end);
> }
>
> void setput (int beg, int cur, int end) {
> setp (pbase () + beg, pbase () + end);
> pbump (cur);
> }
> };
>
> {
> Buf buf ("abcde", std::ios::in);
> buf.setget (1, 2, 4);
> std::printf ("%s\n", buf.str ().c_str ());
> assert ("bcd" == buf.str ());
> }
> {
> Buf buf ("abcde", std::ios::out);
> buf.setput (1, 2, 4);
Here setput() doesn't change the highmark, because highmark
points past to the
last initialized character (in this case points past the 'e') , and
buf.str() will return "bcde".
> std::printf ("%s\n", buf.str ().c_str ());
> assert ("bcd" == buf.str ());
> }
> }
Farid.
Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
Posted by Martin Sebor <se...@roguewave.com>.
Farid Zaripov wrote:
[...]
> The basic_stringbuf<>::str (const char_type *, _RWSTD_SIZE_T)
> have the assertions at begin and at end (sstream.cc line 72 and 167),
> and
> both assertions passes.
Ah, yes, thanks for the pointer! I was looking at the other
two overloads in the header that just call the one in the
.cc file and didn't realize that that one was the workhorse.
>
> The bug is in that len calculated as highmark - pbase(), but pbase()
> == 0
> when buffer opened only in input mode. The begin of the buffer is always
> this->_C_buffer with any openmode. So len = highmark - _C_buffer is
> correct, I think.
In input mode (only) the function is supposed to return:
string(eback(), egptr());
In output mode (or input | output) the function must return:
string(pbase(), high_mark);
So unless I'm missing something the program below should be
a valid (albeit incomplete) test case. Let me know what you
think.
#include <cassert>
#include <cstdio>
#include <sstream>
int main ()
{
struct Buf: std::stringbuf {
Buf (std::string s, std::ios::openmode m)
: std::stringbuf (s, m) { }
void setget (int beg, int cur, int end) {
setg (eback () + beg, eback () + cur, eback () + end);
}
void setput (int beg, int cur, int end) {
setp (pbase () + beg, pbase () + end);
pbump (cur);
}
};
{
Buf buf ("abcde", std::ios::in);
buf.setget (1, 2, 4);
std::printf ("%s\n", buf.str ().c_str ());
assert ("bcd" == buf.str ());
}
{
Buf buf ("abcde", std::ios::out);
buf.setput (1, 2, 4);
std::printf ("%s\n", buf.str ().c_str ());
assert ("bcd" == buf.str ());
}
}
Martin
RE: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
Posted by Farid Zaripov <Fa...@epam.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Friday, May 25, 2007 4:17 AM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV
> in istringstream::str()
>
> Farid Zaripov wrote:
> >> -----Original Message-----
> >> From: Martin Sebor (JIRA) [mailto:jira@apache.org]
> >> Sent: Thursday, May 24, 2007 1:14 AM
> >> To: stdcxx-dev@incubator.apache.org
> >> Subject: [jira] Created: (STDCXX-427) SIGSEGV in
> istringstream::str()
> >>
> > The bug also reproduced on MSVC. The proposed patch below:
>
> Thanks for looking into it!
>
> I'm not sure your proposed patch is correct or necessary
> (although I'm not saying the original code is correct either).
> In fact, I'm not sure the other overload of str(), i.e., the
> one that sets the string buffer, is correct either and I
> suspect that might be the root of the problem.
> The function should assert(this->_C_is_valid()) as a postcondition
> (and the overload below should do the same as a precondition) to
> verify that the buffer is in a stable and consistent state.
> Let's see if the assertion passes first before considering
> this patch further.
The basic_stringbuf<>::str (const char_type *, _RWSTD_SIZE_T)
have the assertions at begin and at end (sstream.cc line 72 and 167),
and
both assertions passes.
The bug is in that len calculated as highmark - pbase(), but pbase()
== 0
when buffer opened only in input mode. The begin of the buffer is always
this->_C_buffer with any openmode. So len = highmark - _C_buffer is
correct, I think.
Farid.
Re: [PATCH] RE: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
Posted by Martin Sebor <se...@roguewave.com>.
Farid Zaripov wrote:
>> -----Original Message-----
>> From: Martin Sebor (JIRA) [mailto:jira@apache.org]
>> Sent: Thursday, May 24, 2007 1:14 AM
>> To: stdcxx-dev@incubator.apache.org
>> Subject: [jira] Created: (STDCXX-427) SIGSEGV in istringstream::str()
>>
>> SIGSEGV in istringstream::str()
>> -------------------------------
>>
>> Key: STDCXX-427
>> URL: https://issues.apache.org/jira/browse/STDCXX-427
>> Project: C++ Standard Library
>> Issue Type: Bug
>> Components: 27. Input/Output
>> Affects Versions: 4.2
>> Environment: gcc-3.3.3 on Linux
>> Reporter: Martin Sebor
>> Priority: Critical
>>
>>
>> This only happens on trunk, not with 4.1.3:
>>
>> $ rm t.o && cat t.cpp && make t && gdb -q t #include
>> <cassert> #include <sstream> #include <string>
>>
>> int main ()
>> {
>> using namespace std;
>>
>> string str ("test");
>>
>> istringstream isstr;
>>
>> isstr.str (str);
>>
>> assert (isstr.str ().length () == str.length ());
>>
>> assert (isstr.str () == str);
>> }
>
> The bug also reproduced on MSVC. The proposed patch below:
Thanks for looking into it!
I'm not sure your proposed patch is correct or necessary
(although I'm not saying the original code is correct either).
In fact, I'm not sure the other overload of str(), i.e., the
one that sets the string buffer, is correct either and I
suspect that might be the root of the problem. The function
should assert(this->_C_is_valid()) as a postcondition (and
the overload below should do the same as a precondition) to
verify that the buffer is in a stable and consistent state.
Let's see if the assertion passes first before considering
this patch further.
Martin
>
> -----------------------
> Index: sstream
> ===================================================================
> --- sstream (revision 541218)
> +++ sstream (working copy)
> @@ -126,7 +126,7 @@
>
> _C_string_type str () const {
> const _RWSTD_SIZE_T __slen = (this->egptr () < this->pptr () ?
> - this->pptr () : this->egptr ()) - this->pbase ();
> + this->pptr () : this->egptr ()) - this->_C_buffer;
> return _C_string_type (this->_C_buffer, __slen);
> }
> -----------------------
>
> Farid.