Re: [conntrack-tools] XML output is invalid

[Patrick McHardy <kaber@trash.net>]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> Does this patch help?
>>>
>>>
>>> +    if (output_mask & _O_XML) {
>>>          op_type = NFCT_O_XML;
>>> +        if (dump_xml_header_done) {
>>> +            dump_xml_header_done = 0;
>>> +            len = snprintf(buf, 1024, "<?xml version=\"1.0\"?>\n"
>>> +                          "<conntrack>\n");
>>> +        }
>>> +    }
>>>      if (output_mask & _O_EXT)
>>>          op_flags = NFCT_OF_SHOW_LAYER3;
>>>      if (output_mask & _O_ID)
>>>          op_flags |= NFCT_OF_ID;
>>>  
>>> -    nfct_snprintf(buf, 1024, ct, NFCT_T_UNKNOWN, op_type, op_flags);
>>> +    nfct_snprintf(buf+len, 1024-len, ct, NFCT_T_UNKNOWN, op_type,
>>> op_flags);
>>
>> It doesn't seem to matter here, but that looks buggy (combined
>> with the snprintf above). When the buffer size is exceed, snprintf
>> returns the amount of characters it *would have written* if
>> enough space was available. So when this really happens above,
>> you have a buffer overflow in the second snprintf.
> 
> The string above has a fixed size and the buffer is big enough to print
> the flow entry, so the buffer overflow is very unlikely.

Yeah, its more a correctness thing. And it might lead to
problems later on.

> Anyhow, I think that the following patch perform more strict and robust
> checkings regarding the buffer size. I hope that you like better :).

Probably, but its missing from this mail :)