On Mon, 2004-12-20 at 03:27, Patrick McHardy wrote:

There are lots of places where this is possible, just look
at all the silly checks in the action code:

    sprintf(act_name, "%s", (char*)RTA_DATA(kind));
    if (RTA_PAYLOAD(kind) >= IFNAMSIZ) {

Hehe. I am sure thats a cutnpaste(LinuxWay) from some code in the kernel
probably sch_api.c (or maybe the code it was cutnpasted has been fixed
in the last 3 years ;->).
That needs fixing. Who is sending the patch?

quoted

Puts the indev validation into its own function to allow
parameter validation before any changes are made. Changes
the sanity check from >= IFNAMSIZ to > IFNAMSIZ to correctly
handle 0 terminated strings and replaces the dangerous sprintf
with a memcpy bound to the TLV size. Providing a indev TLV
for kernels without CONFIG_NET_CLS_IND support will now lead
to EOPPNOTSUPP.

Why special-case indev ? Neither u32 nor fw make any attempt
to clean up after errors in their init functions, so instead
of fixing a single attribute they need to do proper cleanup,
than we can just continue to validate indev when changing it.
Returning EOPNOTSUPP makes sense, of course.

Indev is going to die - no need in investing a lot of effort in it.

I'm also against keeping all those printks when touching the
code. Its ok when writing new code, but I don't see why this
code, unlike everything else, needs to report errors in the
ringbuffer instead of returning meaningful error codes.

Its not that expensive since done on config path. But agree when proper
codes exist its not needed.

cheers,
jamal