Hi Phil,
I am porting your changes to my patch and I have few comments, please
see below.
On 7/20/19 8:52 PM, Phil Sutter wrote:
> Albeit a bit too enthusiastic, gcc is right in that these strings may be
> truncated since the destination buffer is smaller than the source one.
> Get rid of the warnings (and the potential problem) by specifying a
> string "precision" of one character less than the destination. This
> ensures a terminating nul-character may be written as well.
>
> Fixes: af00174af3ef4 ("src: osf: import nfnl_osf.c to load osf fingerprints")
> Signed-off-by: Phil Sutter <phil@xxxxxx>
> ---
> src/nfnl_osf.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/nfnl_osf.c b/src/nfnl_osf.c
> index be3fd8100b665..bed9ba64b65c6 100644
> --- a/src/nfnl_osf.c
> +++ b/src/nfnl_osf.c
> @@ -289,32 +289,34 @@ static int osf_load_line(char *buffer, int len, int del,
> pend = nf_osf_strchr(pbeg, OSFPDEL);
> if (pend) {
> *pend = '\0';
> - cnt = snprintf(obuf, sizeof(obuf), "%s,", pbeg);
> + i = sizeof(obuf);
> + cnt = snprintf(obuf, i, "%.*s,", i - 2, pbeg);
> pbeg = pend + 1;
> }
>
> pend = nf_osf_strchr(pbeg, OSFPDEL);
> if (pend) {
> *pend = '\0';
> + i = sizeof(f.genre);
> if (pbeg[0] == '@' || pbeg[0] == '*')
> - cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg + 1);
> - else
> - cnt = snprintf(f.genre, sizeof(f.genre), "%s", pbeg);
> + pbeg++;
> + cnt = snprintf(f.genre, i, "%.*s", i - 1, pbeg + 1);
> pbeg = pend + 1;
> }
I am not including this because the pbeg pointer is being modified if
the condition is true which is not what we want. Note that pbeg is being
used below. Also, we cannot do pbeg++ and at the same time shift the
pointer passed to snprintf with pbeg + 1.
I propose to let the if statement as it is and modify only the snprintf().
What do you think? Am I missing something here? Thanks Phil!
>
> pend = nf_osf_strchr(pbeg, OSFPDEL);
> if (pend) {
> *pend = '\0';
> - cnt = snprintf(f.version, sizeof(f.version), "%s", pbeg);
> + i = sizeof(f.version);
> + cnt = snprintf(f.version, i, "%.*s", i - 1, pbeg);
> pbeg = pend + 1;
> }
>
> pend = nf_osf_strchr(pbeg, OSFPDEL);
> if (pend) {
> *pend = '\0';
> - cnt =
> - snprintf(f.subtype, sizeof(f.subtype), "%s", pbeg);
> + i = sizeof(f.subtype);
> + cnt = snprintf(f.subtype, i, "%.*s", i - 1, pbeg);
> pbeg = pend + 1;
> }
>
>
