DataFlash LogStructure can have its fields not null terminated

[OXINARF]

Found by Coverity Scan (CID 126737)

In https://github.com/ArduPilot/ardupilot/blob/2c8a0a912310bb03ffc2af5a1e69f09e9f7feb39/libraries/DataFlash/DataFlash.cpp#L402-L404 there's the possibility that strings in the log_write_fmt struct will have sizes bigger than their destinations allow.

In my opinion these 3 lines should be changed to:

strncpy((char*)logstruct.name, f->name, sizeof(logstruct.name) - 1);
strncpy((char*)logstruct.format, f->fmt, sizeof(logstruct.format) - 1);
strncpy((char*)logstruct.labels, f->labels, sizeof(logstruct.labels) - 1);
logstruct.name[4] = '\0';
logstruct.format[15] = '\0';
logstruct.labels[63] = '\0';  // these will need to be memset if we want to keep const

[lucasdemarchi]

Agreed, but 4, 15, and 63 above can be replaced with sizeof(...) - 1

[OXINARF]

@lucasdemarchi It was 4h30 AM, you are asking too much of me :smile:

[amilcarlucas]

@peterbarker does your units patch fix this ?

[peterbarker]

On Sun, 22 Oct 2017, Amilcar Lucas wrote:

@peterbarker does your units patch fix this ?

No.

[WickedShell]

I think it's worth pointing out that requiring it to be null terminated limits you to 15 fields in a message, and I think that's an unfortunate step backwards. Having to handle the null terminator isn't significantly different then what a GCS already has to do with every MAVLink string.

[OXINARF]

I think it's worth pointing out that requiring it to be null terminated limits you to 15 fields in a message

@WickedShell No, it doesn't, we can just increase the size of format and labels to account for the null termination, just like the name already accounts for it (although the code isn't using it).

[WickedShell]

@OXINARF no you can't, not without breaking all the existing log parsers. Everything is loaded via the format spec and a log parser starts with just the top level FMT descriptor preloaded. Extending the length means rewriting all parsers/tools that operate on logs.

[OXINARF]

@WickedShell You are mixing the LogStructure with the log_Format.

[OXINARF]

@WickedShell By the way, a correctly written parser should be able to update the FMT message format too since that is the first message written in the log - only the first message needs to keep the old format.

[WickedShell]

Ah right, sorry has mixed them up.

I agree with you on the log parser, with the caveat that you are still locked to the same symbol table for field lengths.