ENT-12033: Made multiple changes to increase robustness of remote file copying #5620
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
larsewi
force-pushed
the
flat
branch
2 times, most recently
from
a998ba0
to
b66d61d
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
larsewi
force-pushed
the
flat
branch
5 times, most recently
from
05005c9
to
2c9ee5c
Compare
|
@cf-bottom Jenkins please :) |
|
Alright, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/11318/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-11318/ |
vpodzime
reviewed
Oct 25, 2024
libcfnet/client_code.c
Outdated
| Log(LOG_LEVEL_INFO, "Source '%s:%s' changed while copying", | ||
| conn->this_server, source); | ||
| close(dd); | ||
| free(buf); |
There was a problem hiding this comment.
It seems to me that you need some more cleanup. Before this and the other failure catch above this one we have
bool w_ok = FileSparseWrite(dd, buf, n_read,
&last_write_made_hole);
and so it would seem to me you need more than close/free but rather the same as the other failure case catch above, though I am not sure about conn->error = true but seems also reasonable.
free(buf);
unlink(dest);
close(dd);
FlushFileStream(conn->conn_info->sd, size - n_wrote_total - n_read);
conn->error = true;
There was a problem hiding this comment.
I'm not sure about the conn->error = true either. As far as I can see, it's not really checked after executing this code.
There was a problem hiding this comment.
It should be quite easy to merge the two cleanup blocks into a single one acting on two possible error/failure cases.
larsewi
force-pushed
the
flat
branch
3 times, most recently
from
9fb6026
to
50ddf63
Compare
larsewi
commented
Oct 28, 2024
vpodzime
previously approved these changes
Oct 29, 2024
libcfnet/client_code.c
Outdated
| Log(LOG_LEVEL_INFO, "Source '%s:%s' changed while copying", | ||
| conn->this_server, source); | ||
| close(dd); | ||
| free(buf); |
There was a problem hiding this comment.
It should be quite easy to merge the two cleanup blocks into a single one acting on two possible error/failure cases.
Some of the parameters were not that obvious. Hence, I added a doc string so that future hackers can save some time trying to understand this madness. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
Added assert to check return code of snprintf in `[Encrypt]CopyRegularFileNet`. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
There was a check to see if the destination filename was larger than `CF_BUFSIZE - 20` when copying files over the network. This makes no sense to me. At first glans, one might think that it has to do with the network protocol. I.e., `GET $SIZE $FILENAME` has to fit in `CF_BUFSIZE`. However, this is the source filename, and it is never sent over the protocol, as far as I can see. Tried to use git blame to see why it was introduced, but that was not of much help. It appears to date back to the very first implmentation of the files promise in 2008. I'll try removing it. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
Removed unnecessary assignment of terminating NULL-byte to buffer. `snprintf` will terminate the buffer. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
We should not rely on `sscanf` not mutating the pointer arguments on a matching failure as this is not specified in the man page. Instead we should check the expected amount of items are successfully matched. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
Added a sanity check to make sure we don't write more than the original filesize aquired by SYNCH ... STAT source. This might happen if the file was changed while copying. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
Changed to code in `[Encrypt]CopyRegularFileNet` to allocate the receive buffer on the stack instead of the heap. There is no reason to have it dynamically allocated since the size is constant. Furthermore, the buffer is allocated and destroyed in the same scope. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
Previously the file was `stat`'ed every N read to detect file changes during transmission. The reason for limiting the calls to stat is probably to make the code more efficient. However, there are reasons to believe that the bug experienced in ENT-12033 is attributed to filechanges during transmission. Hence, I'm changing the code to do this for every read, as it better to be safe and happy rather than fast but sorry. Yes, `stat()` is a system call. However, it has pretty good cashing mechanisms on modern systems. So it should not be too expensive. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
By flushing the filestream, we make sure that there is no junk data after error. Ticket: None Changelog: None Signed-off-by: Lars Erik Wik <[email protected]>
craigcomstock
approved these changes
Oct 29, 2024
vpodzime
changed the title
vpodzime
approved these changes
Oct 30, 2024
| unlink(dest); | ||
| close(dd); | ||
| FlushFileStream(conn->conn_info->sd, size - n_wrote_total - n_read); | ||
| FlushFileStream(conn->conn_info->sd, size - n_wrote_total); |
There was a problem hiding this comment.
Doesn't this depend on which error case we are handling? I'd probably just check n_wrote_total + n_read in the condition instead of doing the addition first.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initially I was trying to reproduce a bug reported in ENT-12033. However, I was not able to reproduce it. While statically analyzing the code, I made some changes to make it more robust.