adjust assert to allow trailing bytes #121
Conversation
|
@samoht it's now very hard to release tcpip since you seem to have the lock on it and have had it for a while... what needs to happen to merge this and unbreak ARM? |
|
This one needs to merge, but I think it shouldn't be an assert. We should raise a proper exception or use some other functional callback to indicate we dropped the packet (in fact, the latter would be good: a |
|
I think due to it being very partial (there are lots of other potential out-of-bounds accesses, such as the split a few lines above), the assert can just be dropped. It would be good to have an interface which does not throw exceptions in a future tcp/ip release. |
all over the code, checking a particular access is not productive. instead, at some point in the future, the parsers should be refactored and optionally produce packet values. the goal is an exception-free TCP/IP module
|
@samoht @avsm if you want to release TCP/IP + mirage, I'd be happy to back out mirage/mirage#390 (and wait to get conduit/tls/entropy for 2.4.7 then). |
|
Let's not roll anything back. I'll cut a point release of tcpip with this fixed as a branch. |
Alternative fix for #121 to avoid raising Assert_failure See also mirage/mirage-net-xen#24
|
v2.3.1 released. Everyone ok with a silent drop of the malformed frame in #122 instead of an assert? |
|
We should at least print a debug message on the console when this happens. |
|
No, we shouldn't. All such messages need to go to some logging service and not to the console (which is a denial of service very easily). |
|
this is superseeded by changes in 2.3.1 / 2.4.1 |
Alternative fix for mirage#121 to avoid raising Assert_failure See also mirage/mirage-net-xen#24
see issue mirage/mirage-net-xen#24