pkg/detach: fix broken Copy() detach sequence #2302
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -11,47 +12,73 @@ var ErrDetach = errors.New("detached from container") | |||
|
|
|||
| // Copy is similar to io.Copy but support a detach key sequence to break out. | |||
| func Copy(dst io.Writer, src io.Reader, keys []byte) (written int64, err error) { | |||
| // if no key sequence we can use the fast std lib implementation | |||
| if len(keys) == 0 { | |||
There was a problem hiding this comment.
I wonder if we shouldn't mention this in containers.conf - disabling the detach keys (almost never used) improving performance by a pretty significant bit could be relevant to folks.
There was a problem hiding this comment.
I would not make any claims without solid numbers. I have not measured this, the main reason I added this was to not have to deal with the case len(keys) == 0. The io.Copy() code has some interface logic to use better methods when available and logically then don't have to compare each byte * number of keys so yes one should assume it is much faster. However is it faster in the context of podman users. Maybe other overhead makes this pretty much not noticeable.
pkg/detach/copy.go
Outdated
| return 0, ErrDetach | ||
|
|
||
| // previous key buffer | ||
| if len(tmpKeyBuf) > 0 { |
There was a problem hiding this comment.
Would it be faster to just do a linear search of buf instead of copying into a separate, smaller buffer?
There was a problem hiding this comment.
Do you mean like store the index of where we are in the middle of the sequence before the next read is done? That sound like good plan. I was to much into we must keep writing the "buffered" sequence when a later key does not match but in that case we can get the correct bytes just out of the input keys slice instead instead of the tmpKeyBuf.
There was a problem hiding this comment.
If you start searching from (index in buffer) - (length of detach sequence) + 1, you should be able to catch the full sequence even if only one character of the sequence was in the most recent block.
The code only could handle the detach sequence when it read one byte at a time. This obviously is not correct and lead to some issues for my automated test in my podman PR[1] where I added some automated tests for detaching and the read part is really undefined and depends on the input side/kernel scheduling on how much we read at once. This is large rework to make the code check for the key sequence across the entire buffer. That is of course more work but it needs to happen for this to work correctly. I guess the only reason why this was never noticed is because normally user detach manually by typing and not in an automated way which is much slower and thus likely reads the bytes one by one. I added new test to actually confirm the behavior. And to ensure this works with various read sizes I made it a fuzz test. I had this running for a while and did not spot any issues there. The old code fails already on the simple test cases. [1] containers/podman#25083 Signed-off-by: Paul Holzinger <[email protected]>
podman exec support detaching early via the detach key sequence. In that case the podman process should exit successfully but the container exec process keeps running. Now I wrote automated test for both podman run and exec detach but this uncovered several larger issues: - detach sequence parsing is broken[1] - podman-remote exec detach is broken[2] - detach in general seems to be buggy/racy, seeing lot of flakes that fail to restore the terminal and get an EIO instead, i.e. "Unable to restore terminal: input/output error" Thus I cannot add tests for now but this commit should at least fix the obvoius case as reported by the user so I like to get this in regardless and I will work through the other issues once I have more time. Fixes containers#24895 [1] containers/common#2302 [2] containers#25089 Signed-off-by: Paul Holzinger <[email protected]>
podman exec support detaching early via the detach key sequence. In that case the podman process should exit successfully but the container exec process keeps running. Now I wrote automated test for both podman run and exec detach but this uncovered several larger issues: - detach sequence parsing is broken[1] - podman-remote exec detach is broken[2] - detach in general seems to be buggy/racy, seeing lot of flakes that fail to restore the terminal and get an EIO instead, i.e. "Unable to restore terminal: input/output error" Thus I cannot add tests for now but this commit should at least fix the obvoius case as reported by the user so I like to get this in regardless and I will work through the other issues once I have more time. Fixes containers#24895 [1] containers/common#2302 [2] containers#25089 Signed-off-by: Paul Holzinger <[email protected]>
The code only could handle the detach sequence when it read one byte at a time. This obviously is not correct and lead to some issues for my automated test in my podman PR[1] where I added some automated tests for detaching and the read part is really undefined and depends on the input side/kernel scheduling on how much we read at once.
This is large rework to make the code check for the key sequence across the entire buffer. That is of course more work but it needs to happen for this to work correctly.
I guess the only reason why this was never noticed is because normally user detach manually by typing and not in an automated way which is much slower and thus likely reads the bytes one by one.
I added new test to actually confirm the behavior. And to ensure this works with various read sizes I made it a fuzz test. I had this running for a while and did not spot any issues there. The old code fails already on the simple test cases.
[1] containers/podman#25083