Not sure how rare this is to trigger, but it happened on one PC consistently when comb

<a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/us

Infinite loop when respons is a multiple of 128 bytes about git-credential-keepassxc HOT 10 CLOSED

frederick888 commented on May 14, 2024

Infinite loop when respons is a multiple of 128 bytes

from git-credential-keepassxc.

Comments (10)

pitdicker commented on May 14, 2024 1

Yes, this is about Windows, no problems for me on Linux.
With the debug logs it hangs reliably after receiving 4 times 128 bytes.
I'll try to work on it for a bit.

from git-credential-keepassxc.

Frederick888 commented on May 14, 2024 1

Music to my ears :) I'll try getting a release out by this weekend.

I'll also bump the buffer size to 2048 or something, which is usually enough if you get 1 or 2 records per request. The buffer is atm on stack so being a tad frugal here :P

from git-credential-keepassxc.

Frederick888 commented on May 14, 2024

I assume we are talking about Windows here?

I'm afraid I haven't got a proper local Windows environment and I don't use
KeePass 2 / KeePassNatMsg, so I'm not sure when / if I'll be able to look into
this problem myself. Though in the meanwhile, I added some debug logs [1] in
read_to_end and I'm wondering if you can try it out [2]?

[1]

git-credential-keepassxc/src/utils.rs

Lines 266 to 272 in dd3b0b9

 #[cfg(debug_assertions)] 

 { 

 debug!("Received {} bytes: {:?}", len, &buf[0..len]); 

 if let Ok(buf_str) = str::from_utf8(&buf[0..len]) { 

 debug!("Received {} chars: {}", buf_str.len(), buf_str); 

 } 

 }

[2] https://github.com/Frederick888/git-credential-keepassxc/wiki/Troubleshooting#debug-logs

from git-credential-keepassxc.

Frederick888 commented on May 14, 2024

I think I probably know the reason then.

Since these network-like streams haven't got an 'end' unless they are
closed, instead of returning 0 or error, read() just blocks when there's
no data.

This actually also impacts Linux. I tried setting BUF_SIZE to 1 [1] and
it started to block every time.

As a workaround, I added a timeout on these streams in [2] and confirmed
this fixed the issue on Linux using KeePassXC. The timeout is 200ms
which I think is appropriate for local streams, but it'd be nice if you
can test it out as well.

PS: It'll be even better if we can use logic like [3] to detect JSON
boundaries here, but it gets tricky due to these streams' duplex nature.
KeePassXC Browser can handle this since it's a long-running process.
Anyway, the workaround looks good enough to me atm.

[1]

git-credential-keepassxc/src/utils.rs

Line 262 in dd3b0b9

const BUF_SIZE: usize = 128;

[2] https://github.com/Frederick888/git-credential-keepassxc/tree/read-timeout
[3]

git-credential-keepassxc/src/utils.rs

Lines 283 to 327 in dd3b0b9

 fn cut_jsons(response: &str) -> Vec<&str> { 

 let mut results = Vec::new(); 

 let matching_symbol = |c: &char| -> Option<char> { 

 match c { 

 '{' => Some('}'), 

 '}' => Some('{'), 

 '[' => Some(']'), 

 ']' => Some('['), 

 '"' => Some('"'), 

 _ => None, 

 } 

 }; 

 let is_symbol = |c: &char| -> bool { matching_symbol(c).is_some() }; 

 let mut stack = Vec::new(); 

 let mut start = 0; 

 let mut current = 0; 

 let mut escape = false; 

 for c in response.chars() { 

 if stack.is_empty() && current > 0 { 

 results.push(&response[start..current]); 

 start = current; 

 } 

 if !escape && c == '\\' { 

 escape = true; 

 current += 1; 

 continue; 

 } 

 if !is_symbol(&c) || escape { 

 escape = false; 

 current += 1; 

 continue; 

 } 

 if !stack.is_empty() && stack.last().unwrap() == &matching_symbol(&c).unwrap() { 

 stack.pop(); 

 } else { 

 stack.push(c); 

 } 

 current += 1; 

 } 

 results.push(&response[start..]); 

 results 

 }

from git-credential-keepassxc.

Frederick888 commented on May 14, 2024

@pitdicker ping? :)

from git-credential-keepassxc.

pitdicker commented on May 14, 2024

Sorry for the slow reply! I learned a few things, but put off writing it down for too long (had to find back all the links...).

Named pipes on Windows apparently can not only be used as just a byte stream, but also in message mode. If the read buffer is not large enough, it will write part of the message and return ERROR_MORE_DATA (link). But KeePassNatMsg explicitly sets up the pipe in byte mode. And KeePassXC uses QT's QlocalSocket, which on first look only supports byte mode.

The difference between git-credential-helper and the browser proxies is the buffer size of 1024*1024 bytes (KeePassXC, keepassxc-proxy-rust, KeePassNatMsg).

Your solution of using a timeout to detect the end of a message makes the most sense to me, otherwise there is always some message size that gives problems. Would it be a good idea to also use the same buffer size?

from git-credential-keepassxc.

Frederick888 commented on May 14, 2024

TIL Windows named pipes have 2 modes! Luckily the client I use supports
only bytes mode [1], so we are not wrong from the get-go, phew!

And I can certainly bump up the buffer size, but as you mentioned, that
may only make the issue happen less often (and more difficult to hunt
down!). So may I know if read-timeout branch already solves your issue,
i.e. you can see Received 128 bytes: xxxx and only 128 bytes in
debug logs?

[1] https://github.com/blackbeam/named_pipe/blob/08b0f362a282803c31a4591f09c59e5c697f73c0/src/lib.rs#L188

from git-credential-keepassxc.

Frederick888 commented on May 14, 2024

By the way if you no longer have the KeePass record that you used to
produce the issue, you can also set BUF_SIZE [1] to 1 to see if it
hangs.

[1]

git-credential-keepassxc/src/utils.rs

Line 267 in 970578e

const BUF_SIZE: usize = 128;

from git-credential-keepassxc.

pitdicker commented on May 14, 2024

Just did a fresh test, the read-timeout branch works perfectly!

from git-credential-keepassxc.

Frederick888 commented on May 14, 2024

Realised that I had to removed the timeout and take the larger buffer
approach since the timeout breaks credential retrieval when the record
requires user approval (by clicking on Allow Selected in a pop-up
dialogue in KPXC).

v0.9.1 coming... lol

from git-credential-keepassxc.

Infinite loop when respons is a multiple of 128 bytes about git-credential-keepassxc HOT 10 CLOSED

Comments (10)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	#[cfg(debug_assertions)]
	{
	debug!("Received {} bytes: {:?}", len, &buf[0..len]);
	if let Ok(buf_str) = str::from_utf8(&buf[0..len]) {
	debug!("Received {} chars: {}", buf_str.len(), buf_str);
	}
	}

	fn cut_jsons(response: &str) -> Vec<&str> {
	let mut results = Vec::new();

	let matching_symbol = \|c: &char\| -> Option<char> {
	match c {
	'{' => Some('}'),
	'}' => Some('{'),
	'[' => Some(']'),
	']' => Some('['),
	'"' => Some('"'),
	_ => None,
	}
	};
	let is_symbol = \|c: &char\| -> bool { matching_symbol(c).is_some() };

	let mut stack = Vec::new();
	let mut start = 0;
	let mut current = 0;
	let mut escape = false;
	for c in response.chars() {
	if stack.is_empty() && current > 0 {
	results.push(&response[start..current]);
	start = current;
	}
	if !escape && c == '\\' {
	escape = true;
	current += 1;
	continue;
	}
	if !is_symbol(&c) \|\| escape {
	escape = false;
	current += 1;
	continue;
	}
	if !stack.is_empty() && stack.last().unwrap() == &matching_symbol(&c).unwrap() {
	stack.pop();
	} else {
	stack.push(c);
	}
	current += 1;
	}
	results.push(&response[start..]);

	results
	}