Hi,
First of all, thanks a lot for your work on click. It really helped me
with my diploma thesis. However, while working with it, I found two
problems. First, there is an error in the click dequeue implementation.
In operator= it reserves too little memory, resulting in _tail being out
of range and subsequently writing to memory it hasn't allocated. The
following small patch to dequeue.cc fixes that:
50c50
< if (reserve(o._n)) {
---
> if (reserve(o._n + 1)) {
Also I found a problem with Packet::shift_data, but in the meantime you
have changed that function yourself and I don't know if that fixes my
problem. I'll just post my solution, based on click 1.5.0, here - see
the attached packet.diff. It's basically replacing a section of code
with a comment describing the problem, so it should be self-explanatory.
sincerely,
Ulf Hermann
Index: packet.cc
===================================================================
--- packet.cc (Revision 8)
+++ packet.cc (Revision 533)
@@ -463,34 +463,18 @@
Packet *
Packet::shift_data(int offset, bool free_on_failure)
{
- if (offset == 0)
+ if (offset == 0) {
return this;
- else if (!shared() && (offset < 0 ? headroom() >= (uint32_t)(-offset) :
tailroom() >= (uint32_t)offset)) {
- WritablePacket *q = static_cast<WritablePacket *>(this);
-
- unsigned char *ether = (unsigned char*)q->ether_header();
- assert(q->data() >= ether);
-
- if (ether) {
- // TODO check headroom() and tailroom()
- memmove(ether + offset, ether, q->length() + q->data() - ether);
- } else {
- memmove(q->data() + offset, q->data(), q->length());
- }
-
-#if CLICK_LINUXMODULE
- struct sk_buff *mskb = q->skb();
- mskb->data += offset;
- mskb->tail += offset;
-#else /* User-space and BSD kernel module */
- q->_data += offset;
- q->_tail += offset;
-# if CLICK_BSDMODULE
- q->m()->m_data += offset;
-# endif
-#endif
- shift_header_annotations(offset);
- return this;
+ //else if (!shared() && (offset < 0 ? headroom() >= (uint32_t)(-offset) :
tailroom() >= (uint32_t)offset)) {
+ //
+ // This idea is principally flawed as there might be a lot of interesting
data in the headroom or tailroom
+ // which we can't just overwrite.
+ // The author obviously thought of the ethernet header and made an attempt
to work around it.
+ // That however, was buggy as it could overwrite an area before the packet,
and by far not enough as
+ // there might be other interesting things in the headroomi (wifi header,
for example).
+ // So the only way to shift data is expensive_uniqueify for now. This step
is valid
+ // as it seems to have worked before for every shared packet and there is no
difference in the data layout
+ // between shared and unique packets.
} else {
int tailroom_offset = (offset < 0 ? -offset : 0);
if (offset < 0 && headroom() < (uint32_t)(-offset))
_______________________________________________
click mailing list
[email protected]
https://amsterdam.lcs.mit.edu/mailman/listinfo/click
