From 99a802a1aaf5d6b5aa8ae558cce67f658df3736b Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Wed, 18 Dec 2024 10:19:27 +0100 Subject: [PATCH 1/2] Now `cf-net get` no longer unlinks original file The command `cf-net get ` now writes to `.cfnew` followed by replacing `` with `.cfnew`. Previously it would unlink and re-create the original file. With the new `"filestream"` protocol, we need the original file in order to compute its signature, so that we can save bandwidth using the RSYNC protocol. Furthermore, unlinking the original file, causes a brief moment when it either does not exist or is incomplete. This can be problematic if other processes depend on it. Ticket: ENT-12511 Changelog: Title Signed-off-by: Lars Erik Wik --- libcfnet/protocol.c | 158 ++++++++++++++++++++++---------------------- 1 file changed, 78 insertions(+), 80 deletions(-) diff --git a/libcfnet/protocol.c b/libcfnet/protocol.c index 37290c2194..54b0b7fa64 100644 --- a/libcfnet/protocol.c +++ b/libcfnet/protocol.c @@ -101,12 +101,11 @@ bool ProtocolGet(AgentConnection *conn, const char *remote_path, perms = (perms == 0) ? CF_PERMS_DEFAULT : perms; - unlink(local_path); - FILE *file_ptr = safe_fopen_create_perms(local_path, "wx", perms); - if (file_ptr == NULL) + char dest[PATH_MAX]; + int ret = snprintf(dest, sizeof(dest), "%s.cfnew", local_path); + if (ret < 0 || (size_t)ret >= sizeof(dest)) { - Log(LOG_LEVEL_WARNING, "Failed to open file %s (fopen: %s)", - local_path, GetErrorStr()); + Log(LOG_LEVEL_ERR, "Truncation error: Path too long (%d >= %zu)", ret, sizeof(dest)); return false; } @@ -115,112 +114,111 @@ bool ProtocolGet(AgentConnection *conn, const char *remote_path, CF_MSGSIZE, remote_path); - int ret = SendTransaction(conn->conn_info, buf, to_send, CF_DONE); + ret = SendTransaction(conn->conn_info, buf, to_send, CF_DONE); if (ret == -1) { Log(LOG_LEVEL_WARNING, "Failed to send request for remote file %s:%s", conn->this_server, remote_path); - unlink(local_path); - fclose(file_ptr); return false; } - /* Use file stream API if it is available */ + bool success = true; + const ProtocolVersion version = ConnectionInfoProtocolVersion(conn->conn_info); if (ProtocolSupportsFileStream(version)) { - fclose(file_ptr); - - char dest[PATH_MAX]; - ret = snprintf(dest, sizeof(dest), "%s.cfnew", local_path); - if (ret < 0 || (size_t)ret >= sizeof(dest)) - { - Log(LOG_LEVEL_ERR, "Truncation error: Path too long (%d >= %zu)", ret, sizeof(dest)); - return false; - } - + /* Use file stream API if it is available */ if (!FileStreamFetch(conn->conn_info->ssl, local_path, dest, perms)) { /* Error is already logged */ - return false; + success = false; } - - Log(LOG_LEVEL_VERBOSE, "Replacing file '%s' with '%s'...", dest, local_path); - if (rename(dest, local_path) == -1) + } + else { + /* Otherwise, use older protocol */ + unlink(dest); + FILE *file_ptr = safe_fopen_create_perms(dest, "wx", perms); + if (file_ptr == NULL) { - Log(LOG_LEVEL_ERR, "Failed to replace destination file '%s' with basis file '%s': %s", dest, local_path, GetErrorStr()); + Log(LOG_LEVEL_WARNING, "Failed to open file %s (fopen: %s)", + dest, GetErrorStr()); return false; } - return true; - } - - /* Otherwise, use old protocol */ + char cfchangedstr[sizeof(CF_CHANGEDSTR1 CF_CHANGEDSTR2)]; + snprintf(cfchangedstr, sizeof(cfchangedstr), "%s%s", + CF_CHANGEDSTR1, CF_CHANGEDSTR2); - char cfchangedstr[sizeof(CF_CHANGEDSTR1 CF_CHANGEDSTR2)]; - snprintf(cfchangedstr, sizeof(cfchangedstr), "%s%s", - CF_CHANGEDSTR1, CF_CHANGEDSTR2); - - bool success = true; - uint32_t received_bytes = 0; - while (received_bytes < file_size) - { - int len = TLSRecv(conn->conn_info->ssl, buf, CF_MSGSIZE); - if (len == -1) - { - Log(LOG_LEVEL_WARNING, "Failed to GET file %s:%s", - conn->this_server, remote_path); - success = false; - break; - } - else if (len > CF_MSGSIZE) + uint32_t received_bytes = 0; + while (received_bytes < file_size) { - Log(LOG_LEVEL_WARNING, - "Incorrect length of incoming packet " - "while retrieving %s:%s, %d > %d", - conn->this_server, remote_path, len, CF_MSGSIZE); - success = false; - break; - } + int len = TLSRecv(conn->conn_info->ssl, buf, CF_MSGSIZE); + if (len == -1) + { + Log(LOG_LEVEL_WARNING, "Failed to GET file %s:%s", + conn->this_server, remote_path); + success = false; + break; + } + else if (len > CF_MSGSIZE) + { + Log(LOG_LEVEL_WARNING, + "Incorrect length of incoming packet " + "while retrieving %s:%s, %d > %d", + conn->this_server, remote_path, len, CF_MSGSIZE); + success = false; + break; + } - if (BadProtoReply(buf)) - { - Log(LOG_LEVEL_ERR, - "Error from server while retrieving file %s:%s: %s", - conn->this_server, remote_path, buf); - success = false; - break; - } + if (BadProtoReply(buf)) + { + Log(LOG_LEVEL_ERR, + "Error from server while retrieving file %s:%s: %s", + conn->this_server, remote_path, buf); + success = false; + break; + } - if (StringEqualN(buf, cfchangedstr, sizeof(cfchangedstr) - 1)) - { - Log(LOG_LEVEL_ERR, - "Remote file %s:%s changed during file transfer", - conn->this_server, remote_path); - success = false; - break; - } + if (StringEqualN(buf, cfchangedstr, sizeof(cfchangedstr) - 1)) + { + Log(LOG_LEVEL_ERR, + "Remote file %s:%s changed during file transfer", + conn->this_server, remote_path); + success = false; + break; + } - ret = fwrite(buf, sizeof(char), len, file_ptr); - if (ret < 0) - { - Log(LOG_LEVEL_ERR, - "Failed to write during retrieval of file %s:%s (fwrite: %s)", - conn->this_server, remote_path, GetErrorStr()); - success = false; - break; + ret = fwrite(buf, sizeof(char), len, file_ptr); + if (ret < 0) + { + Log(LOG_LEVEL_ERR, + "Failed to write during retrieval of file %s:%s (fwrite: %s)", + conn->this_server, remote_path, GetErrorStr()); + success = false; + break; + } + + received_bytes += len; } - received_bytes += len; + fclose(file_ptr); } if (!success) { - unlink(local_path); + Log(LOG_LEVEL_VERBOSE, "Removing file '%s'...", dest); + unlink(dest); + return false; } - fclose(file_ptr); - return success; + Log(LOG_LEVEL_VERBOSE, "Replacing file '%s' with '%s'...", dest, local_path); + if (rename(dest, local_path) == -1) + { + Log(LOG_LEVEL_ERR, "Failed to replace destination file '%s' with basis file '%s': %s", dest, local_path, GetErrorStr()); + return false; + } + + return true; } bool ProtocolStatGet(AgentConnection *conn, const char *remote_path, From cc1234479a3f711a68ff771e364d6c87d4214541 Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Thu, 19 Dec 2024 11:07:19 +0100 Subject: [PATCH 2/2] File Stream API now unlinks before open with `O_EXCL` The File Stream API now unlinks the destination file (i.e., `.cfnew`) before opening it with the `O_EXCL` flag. Previously the agent would fail if the destination file already exists. Fortunately, the File Stream API unlinks this file afterwards, both on success and error, causing the agent to recover. Both the `cf-net get ` command and the `copy_from` attribute were affected. Ticket: None Changelog: Commit Signed-off-by: Lars Erik Wik --- libcfnet/file_stream.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libcfnet/file_stream.c b/libcfnet/file_stream.c index b840e8edca..a1fd3153b6 100644 --- a/libcfnet/file_stream.c +++ b/libcfnet/file_stream.c @@ -853,6 +853,7 @@ static bool RecvDelta( char in_buf[PROTOCOL_MESSAGE_SIZE * 2], out_buf[PROTOCOL_MESSAGE_SIZE]; /* Open/create the destination file */ + unlink(dest); int new = safe_open_create_perms( dest, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL | O_BINARY, perms); if (new == -1)