Hi Frank,
Frank Kaiser wrote:
I prefer to fix the root cause. However my attempt outlined below did not work, since it does not take into account that the function writes a ‘\0’ at the end of the destination string (something the standard C library function doesn’t do), for which the calculated /size/ value has to be adjusted. The final fix of /Genode::strncpy()/ is:
Indeed. The libc version gives no indication of whether the string was cut or not. So you would need to check dst[size - 1] == 0 for the zero padding (which our version does not implement). So we decided to ensure that the result of the function is always a properly terminated string.
The strncpy function is only a part of a bigger problem, which is the reason why we deferred the fix until now. The root of the problem is that the end of a data spaces acquired from core's ROM service cannot be expected to be padded with zeros. In the corner case of a data module with the exact size of 4096 bytes, there is no padding at all. However, we mistakenly specified the local address of the mapped dataspace directly to the Xml_node constructor for parsing the config file. The constructor, however, expected a null-terminated string. Our fix introduces a further constructor argument for specifying the maximum length of the string. The proper handling of respecting this boundary needed code changes in the XML parser, the tokenizer, and some string functions (e.g., ascii_to_ulong). The strncpy function is final element in the chain of troublemakers ;-)
Looking from the implementation viewpoint, the strncpy function actually complied to the function interface and was not buggy. The interface expects a string as argument 'src', which is, by definition, null- terminated. The size argument is normally used to specify the boundary of the 'dst' buffer, which worked correctly. The problem is that strncpy is called with an invalid 'src' argument and the implicit assumption that the function will not touch memory beyond 'src + size - 1'. However, we need this semantics for our particular data-space-parsing use-case.
I have checked in the complete fix into our SVN. For strncpy, I went for a single loop rather than two loops (checking the size and memcpy) and I think that the resulting code is more obvious. In the process, I also complemented the documentation with regard to the differences between our implementation and the libc version.
Best regards Norman