cf-serverd: Fix format-truncation warning

[btriller]

No description provided.

[larsewi]

Thanks @btriller 🚀 Could you elaborate a little bit on the warning you got? Was it in server_common.c:AbortTransfer?

[btriller]

Thanks @btriller 🚀 Could you elaborate a little bit on the warning you got? Was it in server_common.c:AbortTransfer?

Yes, I got

server_common.c: In function ‘CfEncryptGetFile’:
server_common.c:378:45: warning: ‘%s’ directive output may be truncated writing up to 4095 bytes into a region of size 4063 [-Wformat-truncation=]
  378 |     snprintf(sendbuffer, CF_BUFSIZE, "%s%s: %s",
      |                                             ^~
......
  647 |                 AbortTransfer(conn_info, filename);
      |                                          ~~~~~~~~
In file included from /usr/include/stdio.h:906,
                 from ./../libntech/libutils/platform.h:71,
                 from ./server_common.h:33,
                 from server_common.c:29:
In function ‘snprintf’,
    inlined from ‘AbortTransfer’ at server_common.c:378:5,
    inlined from ‘CfEncryptGetFile’ at server_common.c:647:17:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:54:10: note: ‘__builtin___snprintf_chk’ output between 34 and 4129 bytes into a destination of size 4096
   54 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   55 |                                    __glibc_objsize (__s), __fmt,
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   56 |                                    __va_arg_pack ());
      |                                    ~~~~~~~~~~~~~~~~~

Should I include the location in the commit message?

[larsewi]

I wonder if that warning would disappear if you added a check to the returned value of snprintf() in AbortTransfer(). Something like:

static void AbortTransfer(ConnectionInfo *connection, char *filename)
{
    Log(LOG_LEVEL_VERBOSE, "Aborting transfer of file due to source changes");

    char sendbuffer[CF_BUFSIZE];
    int ret = snprintf(sendbuffer, CF_BUFSIZE, "%s%s: %s",
                       CF_CHANGEDSTR1, CF_CHANGEDSTR2, filename);
    if (ret < 0 || (size_t) ret >= CF_BUFSIZE)
    {
        Log(LOG_LEVEL_WARNING,
            "Error message truncated in abort transfer transaction: "
            "Filename too long");
    }

    if (SendTransaction(connection, sendbuffer, 0, CF_DONE) == -1)
    {
        Log(LOG_LEVEL_VERBOSE, "Send failed in GetFile. (send: %s)",
            GetErrorStr());
    }
}

I don't think a potential truncation of the filename is that critical. The only important part (as far as I know) is that the transaction starts with BAD: for the network protocol to work.

Could you try that?

[larsewi]

If that does not work. Maybe you could add a comment next to the hard coded value, explaining why it is there?

[btriller]

I wonder if that warning would disappear if you added a check to the returned value of snprintf() in AbortTransfer(). Something like:

static void AbortTransfer(ConnectionInfo *connection, char *filename)
{
    Log(LOG_LEVEL_VERBOSE, "Aborting transfer of file due to source changes");

    char sendbuffer[CF_BUFSIZE];
    int ret = snprintf(sendbuffer, CF_BUFSIZE, "%s%s: %s",
                       CF_CHANGEDSTR1, CF_CHANGEDSTR2, filename);
    if (ret < 0 || (size_t) ret >= CF_BUFSIZE)
    {
        Log(LOG_LEVEL_WARNING,
            "Error message truncated in abort transfer transaction: "
            "Filename too long");
    }

    if (SendTransaction(connection, sendbuffer, 0, CF_DONE) == -1)
    {
        Log(LOG_LEVEL_VERBOSE, "Send failed in GetFile. (send: %s)",
            GetErrorStr());
    }
}

I don't think a potential truncation of the filename is that critical. The only important part (as far as I know) is that the transaction starts with BAD: for the network protocol to work.

Could you try that?

Yes, that works too.

[larsewi]

@cf-bottom Jenkins please :)

[cf-bottom]

Alright, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/11919/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-11919/

[craigcomstock]

The change seems OK.

The commits should be squashed for sure.

Can you describe more about what the original "problem" was? Like an example, log snippet?