Filenames with newlines aren't handled properly #695

New issue

Closed

opened 2022-03-23 21:51:52 -05:00 by systwi-again · 9 comments

systwi-again commented 2022-03-23 21:51:52 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

When I use md5sum (version 9.0 from GNU coreutils), I can properly calculate and verify hashes of files containing 0x0A in their filenames:

$ md5sum -- 'foo
bar' |tee md5.txt
\6d573d7359721f7ee1274e75fc4fd21a  foo\nbar
$ md5sum -cw --strict -- md5.txt
\foo\nbar: OK

Unfortunately, this intended behaviour cannot be replicated with xxhsum (version 0.8.1):

$ xxhsum -- 'foo
bar' | tee xxh64.txt
5a651732ba4018ed  foo
bar
$ xxhsum -c --strict --warn xxh64.txt
xxh64.txt:1: Could not open or read 'foo': No such file or directory.
xxh64.txt:2: Error: Improperly formatted checksum line.
1 line is improperly formatted
1 listed file could not be read

Filenames containing carriage returns (0x0D) act bizarrely when quoted (finished with tab complete):

$ xxhsum -- 'foo
bar' | tee xxh64.txt
Error: Could not open 'foo
bar': No such file or directory.

as well as when unquoted:

$ xxhsum -- foo^Mbar # my terminal displays '0x0D' as '^M'
bar13e00c0d7ef4c  foo
$ xxhsum -c --strict --warn xxh64.txt
bar: OK

Notice the 'bar' overlapped part of the hash in my terminal and makes it look like a file named 'bar' has a matching hash. GNU coreutils' md5sum uses \r instead of the raw character (excluding its -z flag).

It seems that the offending character(s) are not backslash-escaped and therefore causes confusion with xxhsum when parsing the list back.

Including/omitting --tag results in the same behaviour.

Tested using Arch GNU/Linux.

When I use `md5sum` (version 9.0 from GNU coreutils), I can properly calculate and verify hashes of files containing `0x0A` in their filenames: $ md5sum -- 'foo bar' |tee md5.txt \6d573d7359721f7ee1274e75fc4fd21a foo\nbar $ md5sum -cw --strict -- md5.txt \foo\nbar: OK Unfortunately, this intended behaviour cannot be replicated with `xxhsum` (version 0.8.1): $ xxhsum -- 'foo bar' | tee xxh64.txt 5a651732ba4018ed foo bar $ xxhsum -c --strict --warn xxh64.txt xxh64.txt:1: Could not open or read 'foo': No such file or directory. xxh64.txt:2: Error: Improperly formatted checksum line. 1 line is improperly formatted 1 listed file could not be read Filenames containing carriage returns (`0x0D`) act bizarrely when quoted (finished with tab complete): $ xxhsum -- 'foo bar' | tee xxh64.txt Error: Could not open 'foo bar': No such file or directory. as well as when unquoted: $ xxhsum -- foo^Mbar # my terminal displays '0x0D' as '^M' bar13e00c0d7ef4c foo $ xxhsum -c --strict --warn xxh64.txt bar: OK Notice the 'bar' overlapped part of the hash in my terminal and makes it look like a file named 'bar' has a matching hash. GNU coreutils' `md5sum` uses `\r` instead of the raw character (excluding its `-z` flag). It seems that the offending character(s) are not backslash-escaped and therefore causes confusion with `xxhsum` when parsing the list back. Including/omitting `--tag` results in the same behaviour. Tested using Arch GNU/Linux.

👍 1 👀 1

t-mat commented 2022-03-24 17:47:54 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Hi @systwi-again, thanks for the report.

I've confirmed your issue. So far, I put terminal agnostic version of your investigation for contributors and future version of our test.

$ # clone

$ cd
$ mkdir xxhsum-issue-695
$ cd xxhsum-issue-695
$ git clone https://github.com/Cyan4973/xxHash.git
$ cd xxHash
$ make
$ mkdir issue-695
$ cd issue-695

$ # create specimen
$ echo issue-695-test-string > $'foo\nbar'

$ # md5sum

$ md5sum -- $'foo\nbar' | tee md5.txt
\c7300c3387ac5a6d439c41122d0e4b43  foo\nbar
$ cat md5.txt
\c7300c3387ac5a6d439c41122d0e4b43  foo\nbar
$ md5sum -cw --strict -- md5.txt
\foo\nbar: OK

$ hexdump -C md5.txt
00000000  5c 63 37 33 30 30 63 33  33 38 37 61 63 35 61 36  |\c7300c3387ac5a6|
00000010  64 34 33 39 63 34 31 31  32 32 64 30 65 34 62 34  |d439c41122d0e4b4|
00000020  33 20 20 66 6f 6f 5c 6e  62 61 72 0a              |3  foo\nbar.|
0000002c

$ # xxhsum

$ ../xxhsum $'foo\nbar' | tee xxh64.txt
901a65c004253f1f  foo
bar
$ cat xxh64.txt
901a65c004253f1f  foo
bar
$ ../xxhsum -c xxh64.txt
xxh64.txt:1: Could not open or read 'foo': No such file or directory.
1 line is improperly formatted
1 listed file could not be read

$ hexdump -C xxh64.txt
00000000  39 30 31 61 36 35 63 30  30 34 32 35 33 66 31 66  |901a65c004253f1f|
00000010  20 20 66 6f 6f 0a 62 61  72 0a                    |  foo.bar.|
0000001a

Hi @systwi-again, thanks for the report. I've confirmed your issue. So far, I put terminal agnostic version of your investigation for contributors and future version of our test. ``` $ # clone $ cd $ mkdir xxhsum-issue-695 $ cd xxhsum-issue-695 $ git clone https://github.com/Cyan4973/xxHash.git $ cd xxHash $ make $ mkdir issue-695 $ cd issue-695 ``` ``` $ # create specimen $ echo issue-695-test-string > $'foo\nbar' ``` ``` $ # md5sum $ md5sum -- $'foo\nbar' | tee md5.txt \c7300c3387ac5a6d439c41122d0e4b43 foo\nbar $ cat md5.txt \c7300c3387ac5a6d439c41122d0e4b43 foo\nbar $ md5sum -cw --strict -- md5.txt \foo\nbar: OK $ hexdump -C md5.txt 00000000 5c 63 37 33 30 30 63 33 33 38 37 61 63 35 61 36 |\c7300c3387ac5a6| 00000010 64 34 33 39 63 34 31 31 32 32 64 30 65 34 62 34 |d439c41122d0e4b4| 00000020 33 20 20 66 6f 6f 5c 6e 62 61 72 0a |3 foo\nbar.| 0000002c ``` ``` $ # xxhsum $ ../xxhsum $'foo\nbar' | tee xxh64.txt 901a65c004253f1f foo bar $ cat xxh64.txt 901a65c004253f1f foo bar $ ../xxhsum -c xxh64.txt xxh64.txt:1: Could not open or read 'foo': No such file or directory. 1 line is improperly formatted 1 listed file could not be read $ hexdump -C xxh64.txt 00000000 39 30 31 61 36 35 63 30 30 34 32 35 33 66 31 66 |901a65c004253f1f| 00000010 20 20 66 6f 6f 0a 62 61 72 0a | foo.bar.| 0000001a ```

👍 2

t-mat commented 2022-03-24 18:52:55 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

How md5sum (digest.c) handles special characters in the filename?

filename_unescape() unescapes the following characters

• \n (0x0a, LF)
• \r (0x0d, CR)
• \\ (0x5c, back slash)

It doesn't accept other \ escaped characters such as \0 or \x7f.
It also doesn't accept NUL (0x00) as a part of filename.

filename_unescape()
https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n547

How md5sum (digest.c) put \ at the beginning of the line?

If filename contains escaped character, digest_check() put \ at the beginning of the line.

https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n1130
https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n1165

---

The following functions also recognize escaped filename.

split_3() , escaped_filename
https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n716

print_filename() , escape
https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n858

output_file() , needs_escape
https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n970

### How `md5sum` (`digest.c`) handles special characters in the filename? `filename_unescape()` unescapes the following characters - `\n` (0x0a, LF) - `\r` (0x0d, CR) - `\\` (0x5c, back slash) It doesn't accept other `\` escaped characters such as `\0` or `\x7f`. It also doesn't accept `NUL` (0x00) as a part of filename. `filename_unescape()` https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n547 ### How `md5sum` (`digest.c`) put `\` at the beginning of the line? If filename contains escaped character, `digest_check()` put `\` at the beginning of the line. https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n1130 https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n1165 --- The following functions also recognize escaped filename. `split_3()` , `escaped_filename` https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n716 `print_filename()` , `escape` https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n858 `output_file()` , `needs_escape` https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/digest.c?id=04d735ea9637b213ef7fc327a635decbbf20152e#n970

👍 2

t-mat commented 2022-03-31 10:39:33 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

I've made experimental branch for this issue - https://github.com/t-mat/xxHash/tree/issue-695

You can check it by the following commands

git clone -b issue-695 https://github.com/t-mat/xxHash xxHash-isshe-695
cd xxHash-isshe-695
make

$ mkdir issue-695
$ cd issue-695/
$ echo issue-965-test-string > $'foo\nbar'
$ ../xxhsum $'foo\nbar' | tee xxh64.txt
\dcf121a176784e70  foo\nbar
$ cat xxh64.txt
\dcf121a176784e70  foo\nbar
$ ../xxhsum -c xxh64.txt
\foo\nbar: OK
$ hexdump -C xxh64.txt
00000000  5c 64 63 66 31 32 31 61  31 37 36 37 38 34 65 37  |\dcf121a176784e7|
00000010  30 20 20 66 6f 6f 5c 6e  62 61 72 0a              |0  foo\nbar.|
0000001c

---

But I have 2 questions

(1) Should we really support \n, \r in the filename / path?

I'm really sorry. And this is 100% my personal taste.
I think we should refuse these filename / path. E.g. just display Error: Failed to open the file.
Because practically we don't gain any benefit from these filenames. I think they just spread confusion and inconvenience.

But since we've been stating and trying to mimic md5sum, we should accept it though.

(2) Should we introduce md5sum style convention to BSD style line?

Since I'm no-brainer, I put same rule to XSUM_printLine_BSD_internal(). But I have absolutely no knowledge about it. I honestly need some help.

I've made experimental branch for this issue - https://github.com/t-mat/xxHash/tree/issue-695 You can check it by the following commands ``` git clone -b issue-695 https://github.com/t-mat/xxHash xxHash-isshe-695 cd xxHash-isshe-695 make ``` ``` $ mkdir issue-695 $ cd issue-695/ $ echo issue-965-test-string > $'foo\nbar' $ ../xxhsum $'foo\nbar' | tee xxh64.txt \dcf121a176784e70 foo\nbar $ cat xxh64.txt \dcf121a176784e70 foo\nbar $ ../xxhsum -c xxh64.txt \foo\nbar: OK $ hexdump -C xxh64.txt 00000000 5c 64 63 66 31 32 31 61 31 37 36 37 38 34 65 37 |\dcf121a176784e7| 00000010 30 20 20 66 6f 6f 5c 6e 62 61 72 0a |0 foo\nbar.| 0000001c ``` --- But I have 2 questions (1) Should we really support `\n`, `\r` in the filename / path? I'm really sorry. And this is 100% my personal taste. I think we should refuse these filename / path. E.g. just display `Error: Failed to open the file`. Because practically we don't gain any benefit from these filenames. I think they just spread confusion and inconvenience. But since we've been stating and trying to mimic `md5sum`, we should accept it though. (2) Should we introduce md5sum style convention to BSD style line? Since I'm no-brainer, I put same rule to `XSUM_printLine_BSD_internal()`. But I have absolutely no knowledge about it. I honestly need some help.

👍 1

Cyan4973 commented 2022-03-31 16:44:32 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

(1) Should we really support \n, \r in the filename / path?

Good question,
I've got essentially the same state of mind :
I would prefer to simplify the situation, even at the cost of forbidding these cases,
but if the goal is to mimic md5sum, and md5sum supports these characters,
then it would be a change of objective.

Hence I presume it's better to support them, though this is weak preference.
If the code becomes massively complex because of that, we might want to revisit that.

(2) Should we introduce md5sum style convention to BSD style line?

Same idea, it depends on md5sum --tag support.
My understanding is that it supports the same rules (regarding "complex" characters) as the regular md5sum output format.

> (1) Should we really support \n, \r in the filename / path? Good question, I've got essentially the same state of mind : I would prefer to simplify the situation, even at the cost of forbidding these cases, but if the goal is to mimic `md5sum`, and `md5sum` supports these characters, then it would be a change of objective. Hence I presume it's better to support them, though this is weak preference. If the code becomes massively complex because of that, we might want to revisit that. > (2) Should we introduce md5sum style convention to BSD style line? Same idea, it depends on `md5sum --tag` support. My understanding is that it supports the same rules (regarding "complex" characters) as the regular `md5sum` output format.

👍 2

t-mat commented 2022-04-02 03:23:33 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

(1) Should we really support \n, \r in the filename / path?
Hence I presume it's better to support them, though this is weak preference.
If the code becomes massively complex because of that, we might want to revisit that.

Sure!

--tag support

Same idea, it depends on md5sum --tag support.
My understanding is that it supports the same rules (regarding "complex" characters) as the regular md5sum output format.

You're right. I've checked md5sum --tag. And your assumption is right.

$ md5sum --tag -- $'foo\nbar' | tee md5-tag.txt
\MD5 (foo\nbar) = c7300c3387ac5a6d439c41122d0e4b43

$ cat md5-tag.txt
\MD5 (foo\nbar) = c7300c3387ac5a6d439c41122d0e4b43

$ md5sum -cw --strict -- md5-tag.txt
\foo\nbar: OK

$ hexdump -C md5-tag.txt
00000000  5c 4d 44 35 20 28 66 6f  6f 5c 6e 62 61 72 29 20  |\MD5 (foo\nbar) |
00000010  3d 20 63 37 33 30 30 63  33 33 38 37 61 63 35 61  |= c7300c3387ac5a|
00000020  36 64 34 33 39 63 34 31  31 32 32 64 30 65 34 62  |6d439c41122d0e4b|
00000030  34 33 0a                                          |43.|
00000033

I also checked xxhsum --tag in my branch. Since it behaves similar, seems okay.

$ ../xxhsum --tag $'foo\nbar' | tee xxh64-tag.txt
\XXH64 (foo\nbar) = dcf121a176784e70

$ cat xxh64-tag.txt
\XXH64 (foo\nbar) = dcf121a176784e70

$ ../xxhsum -c xxh64-tag.txt
\foo\nbar: OK

$ hexdump -C xxh64-tag.txt
00000000  5c 58 58 48 36 34 20 28  66 6f 6f 5c 6e 62 61 72  |\XXH64 (foo\nbar|
00000010  29 20 3d 20 64 63 66 31  32 31 61 31 37 36 37 38  |) = dcf121a17678|
00000020  34 65 37 30 0a                                    |4e70.|
00000025

--zero

It seems filename escaping has relation with --zero. Their web (info) manual of md5sum has the following note.

Without --zero, if file contains a backslash, newline, or carriage return, the line is started with a backslash, and each problematic character in the file name is escaped with a backslash, making the output unambiguous even in the presence of arbitrary file names.

-z
--zero
Output a zero byte (ASCII NUL) at the end of each line, rather than a newline. This option enables other programs to parse the output even when that output would contain data with embedded newlines. Also file name escaping is not used.

So, should we also introduce this feature?

Personally, I'd like to leave it for now. And wait for actual application program which take a benefit from this feature.

> > (1) Should we really support \n, \r in the filename / path? > Hence I presume it's better to support them, though this is weak preference. > If the code becomes massively complex because of that, we might want to revisit that. Sure! ## `--tag` support > Same idea, it depends on md5sum --tag support. > My understanding is that it supports the same rules (regarding "complex" characters) as the regular md5sum output format. You're right. I've checked `md5sum --tag`. And your assumption is right. ``` $ md5sum --tag -- $'foo\nbar' | tee md5-tag.txt \MD5 (foo\nbar) = c7300c3387ac5a6d439c41122d0e4b43 ``` <details> ``` $ cat md5-tag.txt \MD5 (foo\nbar) = c7300c3387ac5a6d439c41122d0e4b43 $ md5sum -cw --strict -- md5-tag.txt \foo\nbar: OK $ hexdump -C md5-tag.txt 00000000 5c 4d 44 35 20 28 66 6f 6f 5c 6e 62 61 72 29 20 |\MD5 (foo\nbar) | 00000010 3d 20 63 37 33 30 30 63 33 33 38 37 61 63 35 61 |= c7300c3387ac5a| 00000020 36 64 34 33 39 63 34 31 31 32 32 64 30 65 34 62 |6d439c41122d0e4b| 00000030 34 33 0a |43.| 00000033 ``` </details> I also checked `xxhsum --tag` in my branch. Since it behaves similar, seems okay. ``` $ ../xxhsum --tag $'foo\nbar' | tee xxh64-tag.txt \XXH64 (foo\nbar) = dcf121a176784e70 ``` <details> ``` $ cat xxh64-tag.txt \XXH64 (foo\nbar) = dcf121a176784e70 $ ../xxhsum -c xxh64-tag.txt \foo\nbar: OK $ hexdump -C xxh64-tag.txt 00000000 5c 58 58 48 36 34 20 28 66 6f 6f 5c 6e 62 61 72 |\XXH64 (foo\nbar| 00000010 29 20 3d 20 64 63 66 31 32 31 61 31 37 36 37 38 |) = dcf121a17678| 00000020 34 65 37 30 0a |4e70.| 00000025 ``` </details> ## `--zero` It seems filename escaping has relation with `--zero`. [Their web (info) manual of `md5sum`](https://www.gnu.org/software/coreutils/manual/html_node/md5sum-invocation.html) has the following note. > Without --zero, if file contains a backslash, newline, or carriage return, the line is started with a backslash, and each problematic character in the file name is escaped with a backslash, making the output unambiguous even in the presence of arbitrary file names. > `-z` > `--zero` > Output a zero byte (ASCII NUL) at the end of each line, rather than a newline. This option enables other programs to parse the output even when that output would contain data with embedded newlines. Also file name escaping is not used. So, should we also introduce this feature? Personally, I'd like to leave it for now. And wait for actual application program which take a benefit from this feature.

👍 1

Cyan4973 commented 2022-04-02 10:24:24 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Personally, I'd like to leave it for now. And wait for actual application program which take a benefit from this feature.

Agreed

> Personally, I'd like to leave it for now. And wait for actual application program which take a benefit from this feature. Agreed

👍 2

t-mat commented 2022-04-03 01:59:37 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

I've added test script to my branch.

make test-filename-escape invokes the test script.

git clone -b issue-695 https://github.com/t-mat/xxHash xxHash-isshe-695
cd xxHash-isshe-695
make
make test-filename-escape

The test script filename-escape.sh invokes the following commands and report there's any error or not.

./xxhsum $'filename-escape-foo\nbar'
./xxhsum --tag $'filename-escape-foo\nbar'

I've added test script to my branch. `make test-filename-escape` invokes the test script. ``` git clone -b issue-695 https://github.com/t-mat/xxHash xxHash-isshe-695 cd xxHash-isshe-695 make make test-filename-escape ``` The test script [`filename-escape.sh`](https://github.com/t-mat/xxHash/blob/ab0b9646ba0dbee4aaecfc8e6b2512f16e34e1ca/tests/filename-escape.sh) invokes the following commands and report there's any error or not. ``` ./xxhsum $'filename-escape-foo\nbar' ./xxhsum --tag $'filename-escape-foo\nbar' ```

👍 2

t-mat commented 2022-04-06 08:50:29 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Hi @systwi-again, since we've merged the fix of this issue, could you check fresh dev branch?
When you'll confirm the resolution. we'd like to close this issue.

Hi @systwi-again, since we've merged the fix of this issue, could you check fresh `dev` branch? When you'll confirm the resolution. we'd like to close this issue.

👍 1

systwi-again commented 2022-04-09 01:14:47 -05:00 (Migrated from github.com)

• Copy link
• Copy Markdown

Thank you @t-mat and @Cyan4973 for looking into this issue. I sincerely apologize for the delay in replying.

The fixes in the dev branch work perfectly under my Arch GNU/Linux install. Thanks again! :)

Thank you @t-mat and @Cyan4973 for looking into this issue. I sincerely apologize for the delay in replying. The fixes in the `dev` branch work perfectly under my Arch GNU/Linux install. Thanks again! :)

👍 2

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

dev

dependabot/github_actions/actions/upload-artifact-7.0.2

dependabot/github_actions/actions/download-artifact-8.0.2

dependabot/github_actions/actions/setup-node-7.1.0

archive/20261011T062114Z-rescue/fhgffy/fix/cmake-dispatch-off

dependabot/github_actions/msys2/setup-msys2-2.33.0

archive/20261011T062114Z-rescue/filtede98/fix-null-guards

ci/release-windows

fix/mingw-exe-symlink

gh-pages

release

ci/pin-actions

docs/refresh-readme

archive/20261011T062114Z-recovered/docs/refresh-readme

v0.8.4

v0.8.3

v0.8.2

v0.8.1

v0.8.0

v0.7.4

v0.7.3

v0.7.2

v0.7.1

v0.7.0

graphs

v0.6.5

v0.6.4

v0.6.3

v0.6.2

v0.6.1

v0.6.0

v0.5.1

v0.5.0

r42

r41

r40

r39

r38

r37

r36

r35

Labels

Clear labels   

announcement

  

bug

  

build

  

CI

Continuous Integration tests

  

dependencies

Pull requests that update a dependency file

  

documentation

  

duplicate

  

enhancement

  

experience feedback

  

feature request

  

github_actions

Pull requests that update GitHub Actions code

  

help wanted

  

install

  

invalid

  

Long-term

  

optimization

  

portability

  

question

  

target-specific

issue triggered in a specific environment (harware, compiler, OS)

  

test issue

  

wontfix

No labels

announcement

bug

build

CI

dependencies

documentation

duplicate

enhancement

experience feedback

feature request

github_actions

help wanted

install

invalid

Long-term

optimization

portability

question

target-specific

test issue

wontfix

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Cyan4973/xxHash#695

No description provided.