nodejs / nodejs/node

fsPromises.cp(...) inconsistencies and bugs

未关闭
#58,869 3 条评论 2 个 reaction 已指派 0 人 在 GitHub 查看

已经有一个关联 PR 被合并了。

  • #58883 来自 @jasnell —— 已合并
confirmed-bug fs
主要语言
JavaScript
星标
122k
派生
37.4k
平均合并
4 天 3 小时
30 天内合并 PR
272

描述

The fsPromises.cp and fs.cp methods are inconsistent with the sync version because they do not correctly accept Buffer file paths. This also means it also improperly handles non-UTF8 encoded filenames. The sync variation supports Buffer.

The method also does not appropriately validate inputs. Rather than throwing a proper Node.js type error when a Buffer is passed, it tries to use it and fails at another point deeper in the function.

The impl of the async version of the method is also needlessly structured differently from the sync version leading to a fair amount of duplicated and inconsistent code, making it difficult to fix the inconsistencies. The sync version was updated recently to move significant pieces to C++ while the async versions were not similarly updated.

Example:

fs.cpSync(Buffer.from('a'), Buffer.from('b'));  // works!

fs.cp(Buffer.from('a'), Buffer.from('b'), (err) => { /* ... */ });  // fails!

The error thrown is:

TypeError [ERR_INVALID_ARG_TYPE]: The "paths[0]" argument must be of type string. Received an instance of Buffer
    at resolve (node:path:1195:7)
    at normalizePathToArray (node:internal/fs/cp/cp:185:45)
    at isSrcSubdir (node:internal/fs/cp/cp:190:18)
    at checkPaths (node:internal/fs/cp/cp:110:32)
    at async cpFn (node:internal/fs/cp/cp:66:17)

fs.cp(...) is currently just a callbackified(...) version of fsPromise.cp(...), so fixing one should fix the other:

There are several fixes necessary: If Buffer is not going to be accepted, then the method should properly validate that the input src and dest are not Buffers and throw a proper ERR_INVALID_ARG_TYPE error. However, not accepting Buffer mean that the async versions of these will not properly handle non-UTF8 encoded file names. Ideally, the method would be updated to accept Buffer paths, but that means the normalizePathToArray method needs to be updated/refactored.

Second, the differences that were introduced when the sync version was updated to move chunks to C++ make it rather difficult to keep these methods in sync with each other. There is a strong possibility/likelihood of inadvertently introducing behavioral differences between the two that increase the likelihood of more inconsistencies being introduced in the future. We should probably move the entire implementation of the cp callback, cp promise, and cp sync methods into C++ if we're going to have any of the implementation in C++, having those eliminate duplicated logic as much as possible.

There is another bug in the implementation of these methods with regards to the options.filter option, which does not correctly handle the case in cpSync when the src and dest are passed as Buffer and when the children of copied directories are not UTF8 encoded file names. The differences in the implementation of the sync and async versions of the cp function make it quite difficult to introduce a consistent fix across the variations. The implementations should be reconciled and moved to C++ first, including the case where options.filter is called, before we can properly fix the options.filter implementation in a consistent way.

@nodejs/fs @dario-piotrowicz @anonrig

贡献指南

打开贡献指南

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

调研方向

从 node:internal/fs/cp/cp 开始,跟踪 normalizePathToArray、cpFn 以及 callback、promise 和 sync 入口点。比较它们对 Buffer 路径和 options.filter 的处理,然后检查 sync 实现所使用的 C++ 路径。当三个变体对 Buffer 和非 UTF-8 路径具有一致的验证和行为,并且 filter 处理相匹配时,即视为完成。

由索引模型根据 Issue 内容生成。

评估

技术栈
cpp, javascript, node.js
领域
backend, operating-systems
Issue 类型
缺陷
难度
5/5
预计耗时
一周以上
活跃度
停滞
描述清晰度
基本清楚
新手友好度
25/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。