System.Console: allow terminfo files to be larger than 4KiB. #82038
Conversation
ghost
added
area-System.Console
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-console Issue DetailsFixes #82026. @adamsitnik @dotnet/area-system-console ptal.
|
| @@ -119,7 +119,9 @@ private static bool TryOpen(string filePath, [NotNullWhen(true)] out SafeFileHan | |||
| { | |||
| // Read in all of the terminfo data | |||
| long termInfoLength = RandomAccess.GetLength(fd); | |||
| const int MaxTermInfoLength = 4096; // according to the term and tic man pages, 4096 is the terminfo file size max | |||
| // according to the term and tic man pages, 4096 is the terminfo file size max | |||
| // Fedora includes terminfo files that are slightly larger than 4096. | |||
There was a problem hiding this comment.
I think the limit mentioned in the man pages is the maximum size of a compiled entry, and not an upper limit for the entire file. This code is then setting an artificial boundary.
We hadn't hit it before, so probably we're not likely to hit it again soon when we double it.
There was a problem hiding this comment.
I don't remember why I did it like this 8 years ago or whatever it was. Is there any reason to have a hard limit at all, rather than just reading/storing until EOF and growing along the way?
There was a problem hiding this comment.
I don't think so, these files are read from 'secure' locations (system or envvar-controlled).
My preference is to remove the limit check.
There was a problem hiding this comment.
if this limit was added for "safety", not only is the location trusted as you say, but also is there anything stopping the file growing between the length check and reading it? Or on Unix does opening a fd (with O_RDONLY as here or otherwise) somehow give you a snapshot view?
There was a problem hiding this comment.
If there were a safety aspect, it'd be more about avoiding OOMs by unbounded memory growth. But I don't think it's an issue. We should just read the full file, regardless of length.
There was a problem hiding this comment.
Right, there's no safety aspect, I just wanted to validate my assumption that there's no way with open() to somehow freeze the size of the file in a totally reliable way.
There was a problem hiding this comment.
Based on our discussion, I've removed the file size check.
|
/backport to release/7.0 |
|
/backport to release/6.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4172292157 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/4172293041 |
|
@adamsitnik backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: System.Console: allow terminfo files to be larger than 4KiB.
Using index info to reconstruct a base tree...
A src/libraries/System.Console/src/System/TermInfo.DatabaseFactory.cs
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/libraries/System.Console/src/System/TermInfo.DatabaseFactory.cs deleted in HEAD and modified in System.Console: allow terminfo files to be larger than 4KiB.. Version System.Console: allow terminfo files to be larger than 4KiB. of src/libraries/System.Console/src/System/TermInfo.DatabaseFactory.cs left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 System.Console: allow terminfo files to be larger than 4KiB.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@adamsitnik an error occurred while backporting to release/6.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
# Conflicts: # src/libraries/System.Console/src/System/TermInfo.DatabaseFactory.cs
ghost
locked as resolved and limited conversation to collaborators
Fixes #82026.
@adamsitnik @dotnet/area-system-console ptal.