Warn on an escaped CR in wsl.conf instead of dropping it silently

[Leo Camus (Dev-next-gen)]

Summary of the Pull Request

Rewritten after review. The original change treated an escaped CRLF as a line continuation; Blue (@OneBlue) ruled that an escaped CR is its own thing and that ending the value at the un-escaped LF is the intended behaviour, and agreed to this narrower change instead.

case '\r': break; made a backslash before a CR the only unrecognised escape the parser accepts silently. Every other one falls into default: and raises MessageConfigInvalidEscape. So in a CRLF file, hostname=f\<CR><LF>oo left hostname as f, dropped the rest, and reported nothing at all — which is how I ran into it, with a truncated value and no diagnostic to explain it.

Removing that case lets an escaped CR warn like any other bad escape. The semantics Blue (@OneBlue) described are unchanged: the value still ends at the un-escaped LF, and as with every other invalid escape the line is then discarded rather than kept truncated.

PR Checklist

• Closes: #xxxx (Replace xxxx with the GitHub issue number)
• Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
• Tests: Added/updated if needed and all pass
• Localization: All end user facing strings can be localized
• Dev docs: Added/updated if needed
• Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Related to #41590, but it does not close it. That issue was filed on the assumption that a CRLF continuation should work; per the review it should not, so the issue is either invalid or wants re-scoping to the missing diagnostic. Happy to close it myself if you'd rather.

No new string is introduced — the existing MessageConfigInvalidEscape resource (Invalid escaped character: '{}' in {}:{}) is reused as-is, so nothing new needs localizing.

The test is one line in WslConfWarnings, placed next to the existing a=a\\m case and built the same way so the line number is unambiguous:

validateWarnings(L"a=a\\\r\nb", L"wsl: Invalid escaped character: '\r' in /etc/wsl.conf:1\r\n");

Validation Steps Performed

I don't have the Windows build environment, so the TAEF suite has not been run and I'm not claiming otherwise.

What I did check: the removed case is the only thing that kept an escaped CR out of default:, so after this change it takes exactly the path the neighbouring a=a\\m case already exercises and passes on. The expected string is derived from localization/strings/en-US/Resources.resw, and the wsl: prefix, the \r\n suffix and the :1 line number are copied from that same passing case rather than guessed.

One thing worth a second opinion, since I can't run it: with the case gone, default: runs goto InvalidLine for a non-matched key, so the whole line is discarded rather than the value being kept as f. That follows from falling through to the default branch, and I read your reply as accepting it, but say the word if you meant to keep the truncated value and only add the warning.

AI tools used

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

A regression test for escaped CRLF continuations is still needed.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates the config parser so escaped CRLF continuations behave like LF continuations.

Changes:

• Consumes \n following an escaped \r.
• Preserves lone \r handling.
• Regression coverage for escaped CRLF continuations is still requested.

File summaries

File	Summary
src/shared/configfile/configfile.cpp	Handles CRLF line continuations; add regression coverage for this case.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The parser fix and regression coverage address the CRLF continuation issue with no unresolved blocking concerns.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Blue (OneBlue)]

I'm a bit confused by this. If we wanted to fully escape a CRLF, shouldn't this be: \\\r\\\n ? Having an un-escaped LF in the value should indeed end parsing for that value so the current behavior seems correct to me.

As a sidenote though, we should probably use CFG_SKIP_INVALID_LINES when we parse wsl.conf

Let me know if I'm missing something.

[Leo Camus (Dev-next-gen)]

You're not missing anything about escaping in general, but I think CRLF is a special case here, and the parser already treats it that way in five other places.

Everywhere else in ParseConfigFile, CRLF is one line terminator rather than two characters:

• L316, comment handling: reads the \r, then consumes the \n that follows.
• L367, start of line: case '\r' looks ahead and only counts the line if the next character is \n, ungetting it otherwise.
• L533: case WEOF: case '\n': case '\r': — CR ends the line exactly like LF.
• L663: the value loop stops on '\n' or '\r', identically.
• L818: the same pair when writing out an invalid line.

The escape switch at L712 is the only place where a \r is consumed without its \n. And it isn't falling through to default: either — case '\r': break; is written out explicitly, so an escaped CR deliberately does not raise MessageConfigInvalidEscape the way any other unknown escape would. The intent to accept a backslash before CRLF already seems to be there; what's missing is swallowing the LF that belongs to the same terminator. That's all this patch does, with the same look-ahead as L316 and L367.

On \\\r\\\n: I don't think that's writable. CR and LF are inserted as a unit when you press Enter, so there's no way to put a backslash between them in an editor. Requiring it would mean line continuations just don't work in any wsl.conf saved with Windows line endings. And the CR really does reach the parser intact — wsl.conf is parsed from src/linux/init/WslDistributionConfig.cpp, so there's no CRT text-mode translation stripping it the way there would be on the Windows side.

On an un-escaped LF ending the value, I agree, and that doesn't change. In hostname=f\<CR><LF>oo the LF isn't un-escaped — it's the back half of the terminator the backslash escaped. A genuinely un-escaped LF still ends the value; the test only covers the continuation.

What happens today with that input is that hostname silently becomes f, and oo is left to be parsed as its own line. That's what I ran into.

Agreed on CFG_SKIP_INVALID_LINES for wsl.conf — happy to do that separately if you want it.

[Blue (OneBlue)]

Everywhere else in ParseConfigFile, CRLF is one line terminator rather than two characters:

• L316, comment handling: reads the \r, then consumes the \n that follows.
• L367, start of line: case '\r' looks ahead and only counts the line if the next character is \n, ungetting it otherwise.
• L533: case WEOF: case '\n': case '\r': — CR ends the line exactly like LF.
• L663: the value loop stops on '\n' or '\r', identically.
• L818: the same pair when writing out an invalid line.

That's true, but these handle un-escaped CRLF. An escaped CR has a different meaning, which is why is handled differented.

On \\\r\\\n: I don't think that's writable. CR and LF are inserted as a unit when you press Enter, so there's no way to put a backslash between them in an editor. Requiring it would mean line continuations just don't work in any wsl.conf saved with Windows line endings. And the CR really does reach the parser intact — wsl.conf is parsed from src/linux/init/WslDistributionConfig.cpp, so there's no CRT text-mode translation stripping it the way there would be on the Windows side.

That depends on the text editor, but a sequence like this can always be written with something like:

$ echo -e '\\\r\n'  > .wslconfig
$ cat -e .wslconfig
\^M$
$

On an un-escaped LF ending the value, I agree, and that doesn't change. In hostname=f\<CR><LF>oo the LF isn't un-escaped — it's the back half of the terminator the backslash escaped. A genuinely un-escaped LF still ends the value; the test only covers the continuation.

I don't agree with the "it's the back half of the terminator the backslash escaped". Doing that would be hostname=f\<CR>\<LF>oo.

[Leo Camus (Dev-next-gen)]

Fair enough on both counts. You can write those bytes with echo -e, so my "not writable" point doesn't hold, and if an escaped CR is its own thing rather than half a terminator, then the five places I pointed at are a different code path and say nothing about it. I'll drop the continuation argument.

What I would still like to fix is narrower and doesn't depend on that reading. \ followed by CR is currently the only unrecognised escape that is accepted silently — every other one falls into default: and raises MessageConfigInvalidEscape. So hostname=f\<CR><LF>oo leaves hostname as f and drops the rest with no diagnostic. That is what I actually hit; I only went looking at the parser because a value came out truncated and nothing said why.

Taking the .gitconfig-style comparison from the file header, git 2.55 does diagnose it. After key = f:

bytes	git
\ LF oo	f
\ CR LF oo	fatal: bad config line 2
\ CR \ LF oo	fatal: bad config line 2

So it rejects the CRLF form rather than truncating quietly, and it rejects the \<CR>\<LF> spelling as well.

Would you take a change that just drops case '\r': break; so an escaped CR falls into default: and warns like any other bad escape? That keeps your semantics exactly — the value still ends at the un-escaped LF — and only replaces a silent truncation with the warning the code already has. I would rewrite this PR to that, with the test changed to assert the warning instead.

If you would rather leave it silent, say so and I will close this. Either way the CFG_SKIP_INVALID_LINES point stands on its own if you want it done.

[Blue (OneBlue)]

Would you take a change that just drops case '\r': break; so an escaped CR falls into default: and warns like any other bad escape? That keeps your semantics exactly — the value still ends at the un-escaped LF — and only replaces a silent truncation with the warning the code already has. I would rewrite this PR to that, with the test changed to assert the warning instead.

Yes I think that would be a good idea.

[Leo Camus (Dev-next-gen)]

Done — force-pushed as dfc8823, the PR is now just the three-line removal plus one WslConfWarnings case:

validateWarnings(L"a=a\\r\nb", L"wsl: Invalid escaped character: '\r' in /etc/wsl.conf:1\r\n");

I rewrote the title and description to match. Two things I flagged in there and would rather raise than leave buried: with the case gone, default: runs goto InvalidLine, so the line is discarded instead of the value staying as f — that comes with falling through, and I read your reply as accepting it, but tell me if you meant to keep the truncated value and only add the warning. And #41590 no longer applies as written, since it was filed assuming the continuation should work; happy to close it or re-scope it to the missing diagnostic, whichever you prefer.

I still can't run the TAEF suite here, so I have not claimed the test passes. The expected string and the :1 are taken from the neighbouring a=a\m case rather than guessed.

[Copilot]

🟡 Changes recommended

The parser handling and regression test do not yet verify the intended CRLF behavior.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/shared/configfile/configfile.cpp:715

• Removing this case makes an escaped \r fall through to default, so a CRLF continuation is still reported as Invalid escaped character: '\r' and parsing aborts. Keep a case '\r' that consumes a following \n (incrementing the line and preserving it in update mode), while pushing back a non-LF character.

            case '\n':
                // Line continuation. Skip both characters.
                line++;
                break;

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Leo Camus (Dev-next-gen)]

On the Copilot review: it is asking for the CRLF continuation to be restored, which is the approach Blue (@OneBlue) ruled against earlier in this thread. An escaped CR falling into default:, warning, and aborting the line is the intended behaviour now, not a regression, so I am leaving that as is.

Its second point made me look harder at my own test though, and it was wrong in a way worth fixing. I had written the content as a=a\<CR><LF>b. That trailing b is a line with no =, so after the escape warning aborts line 1 the parser would reach it and emit a second warning of its own, and validateWarnings compares the whole string. Dropping it, now pushed as 88d2308:

validateWarnings(L"a=a\\\r\n", L"wsl: Invalid escaped character: '\r' in /etc/wsl.conf:1\r\n");

I don't think the test passes on the unfixed parser the way Copilot suggests. Before this change case '\r': break; swallows the CR without a diagnostic, the un-escaped LF ends the value, and the key is accepted — so the warnings string is whatever that produces, not Invalid escaped character: '\r'. That is reasoning from the code rather than a run, since I still have no TAEF environment here, so if you want it confirmed before merging that is the one thing I cannot give you myself.

(My previous comment lost a backslash when I posted it — the code block showed a=a\\r\nb. The branch always had the right escaping.)

[Copilot]

🟢 Approval recommended

The parser change and regression test address the described behavior with no unresolved blocking issues.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Blue (OneBlue)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Blue (OneBlue)]

LGTM, thank you !