Bugzilla – Bug 35
The Packet::AddTrailer function tries to write past the end of the buffer
Last modified: 2008-07-01 13:32:14 EDT
I've included a copy of the function (at the end) for reference. It adds a chuck of memory to the buffer so that it is big enough to serialize the trailer, but then it sets the iterator to the end of the buffer instead of "size of trailer" bytes before the end of the buffer. I think that the patch will be to add a line following: Buffer::Iterator end = m_buffer.End (); end -= size; I will try to generate a patch and test it briefly. Emmanuelle template <typename T> void Packet::AddTrailer (T const &trailer) { NS_ASSERT_MSG (dynamic_cast<Trailer const *> (&trailer) != 0, "Must pass Trailer subclass to Packet::AddTrailer"); uint32_t size = trailer.GetSize (); m_buffer.AddAtEnd (size); Buffer::Iterator end = m_buffer.End (); trailer.Serialize (end); }
Fix needs a small change. The added line should be: end.Prev(size). Emmanuelle
This is not a bug: it is the expected behavior. The iterator fed to SerializeTo is expected to be the "end" of the buffer for a Trailer to mirror the behavior of SerializeTo for a Header and I believe that the API doc in trailer.h and packet.h correctly document this behavior. This was aactually a bug in an earlier version of the code, see changeset 37bb15dd01b1: "hg log -p -r 37bb15dd01b1"
You mean that I have to move the iterator in the SerializeTo function of the derived trailer class? I think that is counterintuitive since you don't need to do that in the SerializeTo function of the header class. But if that is how it is supposed to be, then ok. Emmanuelle
Ok, here is the complete rationale: 1) a trailer is not necessarily written starting from the "start", that is, the first byte. It could be written starting from the "end", that is, the last byte of the packet and if you were to do this, this behavior would be quite natural. It does happen that _some_ protocols such as ATM are logically structured starting from the end of the packet so it is quite natural to do what we do for such protocols. 2) it is quite possible to imagine a Trailer whose length cannot determined before parsing it: all you would need to get into such a case would be a set of trailer options. In that case, it would be impossible to make the calling code correctly rewind the iterator to the start (the first byte) of the trailer. So, the only way to make this case work is by avoiding the rewinding and allowing the user to do it himself if he knows he can do it. 3) there is a certain "naturality" from an implementation perspective to do this. I can understand that this "naturality" could be lost to the user but it is really there if you spend enough time staring at the implementation. Yes, I know, this is a totally subjective rationale. I will try to add a small rationale to the dox doc.
Explained like that, it makes sense to me. I would just add a note to the Doxygen documentation that says the end of the buffer really means the end of the buffer and not the end of the buffer in terms of used bytes (I thought that it meant this is the end of the used buffer, you can start writing here). Or perhaps mention that the iterator needs to be moved before any byte can be written. Emmanuelle
(In reply to comment #5) > Explained like that, it makes sense to me. I would just add a note to the > Doxygen documentation that says the end of the buffer really means the end of > the buffer and not the end of the buffer in terms of used bytes (I thought that > it meant this is the end of the used buffer, you can start writing here). Or > perhaps mention that the iterator needs to be moved before any byte can be > written. I have just tried to do this. Feel free to try to improve the dox text yourself with another patch. > > Emmanuelle >