| Trolltech Home | Qt-interest Home | Recent Threads | All Threads | Author | Date | |
| All threads index page 1 | |
I have some code that I'm building against Qt 4.3.2
under both Linux with g++ 4.2.1 and Win32 with VS2005.
I have run into a problem with QUrl::toEncoded() under
Win32. The following code:
QUrl url("http://news.bbc.co.uk/";);
std::cout << url.toEncoded().data() << std::endl;
emits http://news.bbc.co.uk/ as expected under both
Linux and Win32.
However, I have a method implemented in a plugin class
with the following signature:
bool pull(const QUrl &url,
QFile &file);
If I call this method with the url constructed as above,
then the following code when run inside the pull method:
std::cout << url.toEncoded().data() << std::endl;
emits http://news.bbc.co.uk/ under Linux as expected, but
unprintable data under Win32.
This looks like a bug to me. Anyone seen anything like it ?
--
[ signature omitted ]
Stephen Collyer wrote:
> I have some code that I'm building against Qt 4.3.2
> under both Linux with g++ 4.2.1 and Win32 with VS2005.
>
> I have run into a problem with QUrl::toEncoded() under
> Win32. The following code:
>
> QUrl url("http://news.bbc.co.uk/";);
> std::cout << url.toEncoded().data() << std::endl;
>
> emits http://news.bbc.co.uk/ as expected under both
> Linux and Win32.
>
> However, I have a method implemented in a plugin class
> with the following signature:
>
> bool pull(const QUrl &url,
> QFile &file);
>
> If I call this method with the url constructed as above,
> then the following code when run inside the pull method:
>
> std::cout << url.toEncoded().data() << std::endl;
>
> emits http://news.bbc.co.uk/ under Linux as expected, but
> unprintable data under Win32.
>
> This looks like a bug to me. Anyone seen anything like it ?
Some more info:
1. In the offending code I'm actually doing this:
char *data = url.toEncoded().data();
std::cout << data << std::endl;
I've taken a look at the doco for QByteArray::data() and
this seems to be safe, though there's a slightly cryptic comment
about using the data as long as "the byte array isn't reallocated".
I can't see that I can be "reallocating" anything in the code above.
2. I've debugged it a bit, and the problem seems to be that at
the end of QByteArray::data() the QByteArray dtor is running, and
this is eventually calling qFree(d) (after a bit of ref. counting
logic) that leaves a dangling pointer. It strikes me that there's
a problem in the ref. counting logic since this seems to break
the "safe to use as long as not reallocated" guarantee.
Any Trolls able to comment ?
--
[ signature omitted ]
Stephen Collyer schrieb:
> Stephen Collyer wrote:
>> I have some code that I'm building against Qt 4.3.2
>> under both Linux with g++ 4.2.1 and Win32 with VS2005.
>>
>> I have run into a problem with QUrl::toEncoded() under
>> Win32. The following code:
>>
>> QUrl url("http://news.bbc.co.uk/";);
>> std::cout << url.toEncoded().data() << std::endl;
>>
>> emits http://news.bbc.co.uk/ as expected under both
>> Linux and Win32.
>>
>> However, I have a method implemented in a plugin class
>> with the following signature:
>>
>> bool pull(const QUrl &url,
>> QFile &file);
>>
>> If I call this method with the url constructed as above,
>> then the following code when run inside the pull method:
>>
>> std::cout << url.toEncoded().data() << std::endl;
>>
>> emits http://news.bbc.co.uk/ under Linux as expected, but
>> unprintable data under Win32.
>>
>> This looks like a bug to me. Anyone seen anything like it ?
>
> Some more info:
>
> 1. In the offending code I'm actually doing this:
>
> char *data = url.toEncoded().data();
> std::cout << data << std::endl;
>
> I've taken a look at the doco for QByteArray::data() and
> this seems to be safe, though there's a slightly cryptic comment
> about using the data as long as "the byte array isn't reallocated".
> I can't see that I can be "reallocating" anything in the code above.
>
Can we have a C++ beginners FAQ for this? Same question every month. :)
The QByteArray create with url.toEncoded() is gone right after the ';'
and therefore data points to nowhere. Write it in one line:
std::cout << url.toEncoded().data() << std::endl;
and all works fine.
Christian
--
[ signature omitted ]
Christian Ehrlicher wrote:
> Can we have a C++ beginners FAQ for this? Same question every month. :)
> The QByteArray create with url.toEncoded() is gone right after the ';'
> and therefore data points to nowhere. Write it in one line:
I've taking the code below directly from the QByteArray documentation:
QByteArray ba("Hello world");
char *data = ba.data();
while (*data) {
cout << "[" << *data << "]" << endl;
++data;
}
This is using the pointer, seemingly with impunity, after that fateful ;
In addition, the documentation states outright:
> The pointer remains valid as long as the byte array isn't reallocated.
> For read-only access, constData() is faster because it never causes
> a deep copy to occur.
>
> This function is mostly useful to pass a byte array to a function that
> accepts a const char *.
Based on this, it seems reasonable to assume that data() returns a
persistent buffer, wouldn't you think ?
This problem seems to be entirely unrelated to C++,
whether practised by a bunglingly incompetent beginner such as
me or by an intellectual colossus such as you; it seems to
be instead an error in the docs.
> std::cout << url.toEncoded().data() << std::endl;
>
> and all works fine.
I need a persistent copy of the char* returned by data()
so I'm afraid all wouldn't work fine. I guess given the
way the implementation has been done, I'll need to allocate
it myself ?
--
[ signature omitted ]
In the example, the QByteArray is on the stack and remains until the closing
} for the block it is in. In your code, you are not allocating a QByteArray,
so it goes out of scope at the ;. This is basic C++, but few of us learn it
until we suffer from it.
Yes, you need to allocate your own buffer, or your own QByteArray.
Keith
**Please do not reply to me, reply to the list.**
On 01-07-2008 4:43 PM, "Stephen Collyer" wrote:
> Christian Ehrlicher wrote:
>
>> Can we have a C++ beginners FAQ for this? Same question every month. :)
>> The QByteArray create with url.toEncoded() is gone right after the ';'
>> and therefore data points to nowhere. Write it in one line:
>
> I've taking the code below directly from the QByteArray documentation:
>
> QByteArray ba("Hello world");
> char *data = ba.data();
> while (*data) {
> cout << "[" << *data << "]" << endl;
> ++data;
> }
>
> This is using the pointer, seemingly with impunity, after that fateful ;
> In addition, the documentation states outright:
>
>> The pointer remains valid as long as the byte array isn't reallocated.
>> For read-only access, constData() is faster because it never causes
>> a deep copy to occur.
>>
>> This function is mostly useful to pass a byte array to a function that
>> accepts a const char *.
>
> Based on this, it seems reasonable to assume that data() returns a
> persistent buffer, wouldn't you think ?
>
> This problem seems to be entirely unrelated to C++,
> whether practised by a bunglingly incompetent beginner such as
> me or by an intellectual colossus such as you; it seems to
> be instead an error in the docs.
>
>> std::cout << url.toEncoded().data() << std::endl;
>>
>> and all works fine.
>
> I need a persistent copy of the char* returned by data()
> so I'm afraid all wouldn't work fine. I guess given the
> way the implementation has been done, I'll need to allocate
> it myself ?
--
[ signature omitted ]
Keith Esau wrote: > In the example, the QByteArray is on the stack and remains until the closing > } for the block it is in. In your code, you are not allocating a QByteArray, > so it goes out of scope at the ;. Keith, yes, you're entirely right. The blasted QByteArray in my code is ephemeral. > This is basic C++, but few of us learn it until we suffer from it. Hey, that's not fair. Some of us suffer from it *long* after we learnt it, in many languages :-) Oh well, panic over. -- [ signature omitted ]
On Mon, 2008-01-07 at 22:43 +0000, Stephen Collyer wrote:
> Christian Ehrlicher wrote:
>
> > Can we have a C++ beginners FAQ for this? Same question every month. :)
> > The QByteArray create with url.toEncoded() is gone right after the ';'
> > and therefore data points to nowhere. Write it in one line:
>
> I've taking the code below directly from the QByteArray documentation:
>
> QByteArray ba("Hello world");
> char *data = ba.data();
> while (*data) {
> cout << "[" << *data << "]" << endl;
> ++data;
> }
There is a DISTINCT difference between *that* code and your code though!
The code above creates a QByteArray on the stack which exists as long as
you don't go out of scope. Hence, the ba.data() pointer is safe to use.
However,
url.toEncoded().data(); only *temporarily* creates a QByteArray, returns
data() and then when the ; hits, it goes out of scope and QByteArray is
lost.
If you want to preserve your QByteArray, do this:
{
QByteArray array = url.toEncoded();
char* data = array.data();
}
At this point in time, your array exists until it goes out of scope
at }. So as long as you use it before that, you will be fine.
>
> This is using the pointer, seemingly with impunity, after that fateful ;
> In addition, the documentation states outright:
>
> > The pointer remains valid as long as the byte array isn't reallocated.
> > For read-only access, constData() is faster because it never causes
> > a deep copy to occur.
> >
> > This function is mostly useful to pass a byte array to a function that
> > accepts a const char *.
>
> Based on this, it seems reasonable to assume that data() returns a
> persistent buffer, wouldn't you think ?
>
> This problem seems to be entirely unrelated to C++,
> whether practised by a bunglingly incompetent beginner such as
> me or by an intellectual colossus such as you; it seems to
> be instead an error in the docs.
>
> > std::cout << url.toEncoded().data() << std::endl;
> >
> > and all works fine.
>
> I need a persistent copy of the char* returned by data()
> so I'm afraid all wouldn't work fine. I guess given the
> way the implementation has been done, I'll need to allocate
> it myself ?
If you need a persistent char* you have to create it from a persistent
object. char* ptr = url.toEncoded().data(); is NOT from a persistent
QByteArray.
Stephan
--
[ signature omitted ]
Stephan Rose wrote: > If you need a persistent char* you have to create it from a persistent > object. char* ptr = url.toEncoded().data(); is NOT from a persistent > QByteArray. Right. This is indeed true. But whereas last night I was ready to grovellingly accept that this was user-error, having re-read the docs for QByteArray, I'll reaffirm my position: the documentation is misleading and ought to be changed. Lets look at what it says: "Returns a pointer to the data stored in the byte array... The pointer remains valid as long as the byte array isn't reallocated" There are several problems here. 1. It's not made explicitly clear that the pointer belongs to the QByteArray object. 2. The terminology is incorrect: the byte array is never "reallocated"; rather, it is freed. To "reallocate" a pointer leads the casual reader to make a connection to realloc(). 3. Because of the reference to reallocation, it's too easy to assume that the data was allocated in the first place. The only real clue that it wasn't is a negative one, namely the absence of an instruction that the pointer must be free'd by the caller. I think all of this can be side-stepped by changing the wording: "The pointer belongs to the QByteArray object in question, and is no longer valid when the object goes out of scope. Users who require a persistent copy of the data must copy the data into a QByteArray of the appropriate scope." or something along those lines. And since I'm feeling much happier this morning, I will refrain from making any reference to Item 29 of Effective C++ Volume 1 ... -- [ signature omitted ]
> Von: Stephen Collyer > Stephan Rose wrote: <snip> > I think all of this can be side-stepped by changing > the wording: > > "The pointer belongs to the QByteArray object in question, > and is no longer valid when the object goes out of scope. > Users who require a persistent copy of the data must copy > the data into a QByteArray of the appropriate scope." > But that's not the full story. It's valid until QByteArray is modified (==reallocated, which also includes destroyed) Christian -- [ signature omitted ]
Christian Ehrlicher wrote: >> >> "The pointer belongs to the QByteArray object in question, >> and is no longer valid when the object goes out of scope. >> Users who require a persistent copy of the data must copy >> the data into a QByteArray of the appropriate scope." >> > But that's not the full story. It's valid until QByteArray is modified (==reallocated, which also includes destroyed) Fair enough. It was only a suggestion. I'm sure Trolltech can come up with more accurate wording. BTW, I recall someone mentioned that this problem arises on a monthly basis. It strikes me that that is a clear indication that documentation could bear improvement. -- [ signature omitted ]
On Tuesday 08 January 2008 10:26:08 Stephen Collyer wrote:
> Stephan Rose wrote:
Hi Stephan.
> > If you need a persistent char* you have to create it from a persistent
> > object. char* ptr = url.toEncoded().data(); is NOT from a persistent
> > QByteArray.
>
> Right. This is indeed true. But whereas last night I was ready to
> grovellingly accept that this was user-error, having re-read the
> docs for QByteArray, I'll reaffirm my position: the documentation
> is misleading and ought to be changed. Lets look at what it says:
>
> "Returns a pointer to the data stored in the byte array...
> The pointer remains valid as long as the byte array isn't
> reallocated"
>
> There are several problems here.
>
> 1. It's not made explicitly clear that the pointer belongs
> to the QByteArray object.
It doesn't "belong" to the object.
The first sentence of the documentation describes pretty
well what is happening:
"Returns a pointer to the data stored in the byte array."
As a corollary, whatever happens to the "data stored in
the byte array" happens to the "data pointed to by the
returned pointer", so if e.g. the data stored in the byte
array gets freed, the pointer will point to freed memory.
> 2. The terminology is incorrect: the byte array is
> never "reallocated"; rather, it is freed. To "reallocate"
> a pointer leads the casual reader to make a connection
> to realloc().
I dispute "incorrect", and I'd even dispute "incomplete".
Nevertheless I added "or destroyed" as reason for the
invalidation in the docs. They read now:
"The pointer remains valid as long as the byte array isn't
reallocated _or destroyed_."
I hope this addresses the discussed problem.
Regards,
Andre'
PS:
> 3. Because of the reference to reallocation, it's too
> easy to assume that the data was allocated in the first
> place.
Playing "Devil's advocate" I tried to read this from the docs
but I really can't.
> The only real clue that it wasn't is a negative
> one, namely the absence of an instruction that the pointer
> must be free'd by the caller.
> I think all of this can be side-stepped by changing
> the wording:
>
> "The pointer belongs to the QByteArray object in question,
> and is no longer valid when the object goes out of scope.
> Users who require a persistent copy of the data must copy
> the data into a QByteArray of the appropriate scope."
That would be worse as the pointer does really not "belong"
to the QByteArray object. There is no member of the object
holding that pointer or such.
--
[ signature omitted ]
André Pönitz wrote: > The first sentence of the documentation describes pretty > well what is happening: > > "Returns a pointer to the data stored in the byte array." > > As a corollary, whatever happens to the "data stored in > the byte array" happens to the "data pointed to by the > returned pointer", so if e.g. the data stored in the byte > array gets freed, the pointer will point to freed memory. I can't fault your logic here, but the problem to my mind is not the first sentence in isolation; it's that the whole section seems to imply that an allocated copy of the data is returned. > Nevertheless I added "or destroyed" as reason for the > invalidation in the docs. They read now: > > "The pointer remains valid as long as the byte array isn't > reallocated _or destroyed_." > > I hope this addresses the discussed problem. If I were writing the documentation, I would explicitly point out that users should avoid writing the kind of code that I came up with, where temporary QByteArrays are constructed. I suspect that would fix the problem for 90% of the people who run into this problem, leaving the 10% who won't understand what's going wrong anyway. > Playing "Devil's advocate" I tried to read this from the docs > but I really can't. Well, fair enough, but I suspect you already know what the function does anyway. I can assure you that when I read the docs last night (and I admitted I only skimmed them, just to make sure that what I was doing was safe), I was in no doubt that I was getting a copy of the data. Clearly I was wrong, but that's how I interpreted the wording. Bear in mind that the data() method is doing one of the most dangerous things you can do in a language with dynamic memory allocation - it's pointing to something that is not going to be valid in a pretty short time, in many scenarios. It's precisely this kind of operation that should be documented as clearly as possible, particularly bearing in mind the warm, fuzzy, comfort zone that the QObject heirarchy offers regarding deallocation of objects. -- [ signature omitted ]
On Tue, 2008-01-08 at 11:05 +0000, Stephen Collyer wrote: > Andrà PÃnitz wrote: > <snip> > > Well, fair enough, but I suspect you already know what the > function does anyway. I can assure you that when I read the > docs last night (and I admitted I only skimmed them, just to > make sure that what I was doing was safe), I was in no doubt > that I was getting a copy of the data. Clearly I was wrong, > but that's how I interpreted the wording. > > Bear in mind that the data() method is doing one of the most > dangerous things you can do in a language with dynamic memory > allocation - it's pointing to something that is not going to be > valid in a pretty short time, in many scenarios. It's precisely > this kind of operation that should be documented as clearly as > possible, particularly bearing in mind the warm, fuzzy, comfort > zone that the QObject heirarchy offers regarding deallocation > of objects. > Stephen, with all due respect, this is a C++ problem, not a Qt Problem. It is not Trolltech's responsibility to provide documentation on the basics of the C++ language. You're going to have this kind of problem *anytime* you're accessing a temporary object after it has gone out of scope, not just with a QByteArray. Stephan -- [ signature omitted ]
Stephan Rose wrote: > You're going to have this kind of problem *anytime* you're accessing a > temporary object after it has gone out of scope, not just with a > QByteArray. The problem, to my mind, is that the documentation gives the impression (or can be read in such a way as to give the impression) that the char * points to data of greater scope than the temporary object, I would prefer it if it didn't. Anyway, it's up to Trolltech to document it as they see fit, and I don't think I can usefully add anything to what I've already said. It's not really a C++ problem, either. The practice of returning a pointer to internal data will be a danger area in any language that allows the construction of ephemeral objects. -- [ signature omitted ]
On Wednesday 09 January 2008 10:45:50 Stephen Collyer wrote:
> Stephan Rose wrote:
> > You're going to have this kind of problem *anytime* you're accessing a
> > temporary object after it has gone out of scope, not just with a
> > QByteArray.
>
> The problem, to my mind, is that the documentation gives the
> impression (or can be read in such a way as to give the impression)
> that the char * points to data of greater scope than the temporary
> object, I would prefer it if it didn't.
I'm sorry, but if you follow that line of thinking, you should have asked the
question: what greater scope? If the owner of that pointer isn't QByteArray,
then who is?
If you had thought it was QUrl, then consider the following code:
QByteArray encoded;
{
QUrl url(".....");
encoded = url.toEncoded();
}
qDebug() << encoded.constData();
The code above must work, right? So it means the pointer isn't owned by QUrl,
because it's already gone out of scope.
Anyways, the point is: unless the documentation explicitly says that it is
transferring ownership of the pointer to you -- and it does so, because you
have to remember to delete the pointer later, using the proper function --
then it is not transferring it. The pointer still belongs to the object and
you may not assume it will outlive the object.
(It *can* outlive if it is created with QByteArray::fromRawData)
> It's not really a C++ problem, either. The practice of returning
> a pointer to internal data will be a danger area in any language
> that allows the construction of ephemeral objects.
See the comment above. Passing pointers around is inherently dangerous, I
agree (I've just fixed a crash in one of our examples because a pointer was
deleted too soon). Using pointers requires proper rules for ownership and
life-cycle management.
--
[ signature omitted ]
Attachment:
signature.asc
Description: This is a digitally signed message part.