nodejs / nodejs/node

fsPromises.cp(...) inconsistencies and bugs

オープン
#58,869 コメント 3 件 リアクション 2 件 担当者 0 名 GitHub で見る

関連するプルリクエストがすでにマージされています。

  • #58883 @jasnell による — マージ済み
confirmed-bug fs
主要言語
JavaScript
スター
122k
フォーク
37.3k
平均マージ
4日 2時間
マージ済み PR(30日)
283

説明

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. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

調査の方向性

node:internal/fs/cp/cp から開始し、normalizePathToArray、cpFn、および callback、promise、sync のエントリポイントを追跡します。それらの Buffer パスと options.filter の処理を比較し、続いて sync 実装で使用される C++ パスを確認します。3 つのバリアントで、Buffer パスおよび非 UTF-8 パスに対する検証と動作が一貫し、filter の処理も一致していれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
cpp, javascript, node.js
領域
backend, operating-systems
issue の種類
バグ
難易度
5/5
見積もり時間
1週間以上
活発さ
停滞
明瞭さ
おおむね明確
初心者へのやさしさ
25/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。