LockData: Limit number of '|' chars that split the message#319
LockData: Limit number of '|' chars that split the message#319derrickstolee wants to merge 1 commit intomicrosoft:masterfrom
Conversation
| string[] dataParts = body.Split(new char[] { MessageSeparator }, count: 5); | ||
| int pid; | ||
| bool isElevated = false; | ||
| bool checkAvailabilityOnly = false; |
There was a problem hiding this comment.
We then complain when the length doesn't match.
Prior to your change, where was VFS for Git complaining that the length didn't match? The change looks OK to to me, but I can't see how the old code was running into problems.
There was a problem hiding this comment.
Below, there is an exception for "Invalid lock message. The parsedCommand is an unexpected length, got: {0} from message: '{1}'".
Note further that we only complaint if dataParts.Length < 5 instead of succeeding in the correct way.
Most of the errors in the logs are off-by-one errors that were solved in #259.
There was a problem hiding this comment.
I did see that check, but I couldn't see how extra | would break it.
As best I could tell parsedCommandLength shouldn't care about extra |, and the check is:
if (<header lengths> + parsedCommandLength) != <entire length of message>:
// ParsedCommandLength should be the length of the string at the end of the message
// Add the length of the previous parts, plus delimiters
int startingSpot = dataParts[0].Length + dataParts[1].Length + dataParts[2].Length + dataParts[3].Length + 4;
if ((startingSpot + parsedCommandLength) != body.Length)
There must be something I'm missing, any ideas?
There was a problem hiding this comment.
Ah you're right! I guess I misread that we get the data from the full body instead of dataParts[4]. Undoing my production code change doesn't make the test fail. I'll close this PR.
There was a problem hiding this comment.
I think the change you made to FromBody does make the code more readable and so if you still wanted to merge in the PR that'd be good with me.
I see we do have a unit test that already validates this scenario (look for // Verify strings with "|" will work) and so I'm not sure you'd need to merge that change in.
2 participants
We send multiple data fields in our lock request and use the pipe symbol '|' to split these fields. However, the final field is free-text, so could include that symbol. We then complain when the length doesn't match.
Limit our splitting to be the expected number of fields, removing this issue. Add a simple unit test that verifies this behavior.