New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

string escape drops NUL and following text #4605

Closed
cben opened this Issue Dec 17, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@cben
Copy link
Contributor

cben commented Dec 17, 2017

fish, version 2.7.0-513-g81dd4a45
Linux, Fedora 27, gnome-terminal (TERM=xterm-256cotor).
Confirmed without customization (sh -c 'env HOME=$(mktemp -d) fish').

Actual behavior: string escape doesn't handle NUL:

/h/b/fish-shell> printf 'foo\0bar' | cat -A
foo^@bar⏎
/h/b/fish-shell> printf 'foo\0bar' | string escape
foo

and string unescape has similar problem:

/h/b/fish-shell> printf 'escaped\\0text' | string unescape
escaped

Expected behavior:

/h/b/fish-shell> printf 'foo\0bar' | string escape
foo\0bar
/h/b/fish-shell> printf 'escaped\\0text' | string unescape | cat -A
escaped^@text⏎

Generally, it should be possible to round-trip any binary data through | string escape | string unescape |.

@floam floam added the bug label Dec 19, 2017

@floam floam added this to the fish-future milestone Dec 19, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 19, 2017

Yup. The issue is that we're still using c-style strings all over the place. This might take a while to solve.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Dec 25, 2017

This is fixable in each individual case - if there's some particular case that's blocking you please let us know.

faho added a commit to faho/fish-shell that referenced this issue Jan 2, 2018

[string] Allow `string escape` to handle NULs
TODO: This is a bit hacky (since it depends on getting the `storage`
wcstring), and it currently only works for the "script" escaping style.

Work towards fish-shell#4605.
@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 2, 2018

I've now implemented this in a branch for the "script" escaping style (see the commit linked above).

Note that it's a bit hacky because it needs to get the wcstring, so I just added a method to get a reference to the arg_iterator_t's storage_, and since I still don't fully grok C++ reference lifetimes that might be not entirely correct.

I want to implement it for the other escape styles as well before I merge it.

faho added a commit to faho/fish-shell that referenced this issue Jan 3, 2018

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 11, 2018

I have now converted all string subcommands except for match and replace. Those are a bit trickier because of the interactions with pcre.

Also, as it turns out escape, unescape, upper and lower have a bunch of duplicated code - the latter two do the exact same thing except one passes "std::towupper" and the other "std::towlower".

faho added a commit to faho/fish-shell that referenced this issue May 11, 2018

[string] Allow `string escape` to handle NULs
TODO: This is a bit hacky (since it depends on getting the `storage`
wcstring), and it currently only works for the "script" escaping style.

Work towards fish-shell#4605.

faho added a commit to faho/fish-shell that referenced this issue May 11, 2018

faho added a commit to faho/fish-shell that referenced this issue May 13, 2018

[string] Allow `string escape` to handle NULs
TODO: This is a bit hacky (since it depends on getting the `storage`
wcstring), and it currently only works for the "script" escaping style.

Work towards fish-shell#4605.

faho added a commit to faho/fish-shell that referenced this issue May 13, 2018

faho added a commit to faho/fish-shell that referenced this issue May 13, 2018

[string] Allow `string escape` to handle NULs
TODO: This currently only works for the "script" escaping style.

Work towards fish-shell#4605.

faho added a commit to faho/fish-shell that referenced this issue May 13, 2018

@faho faho referenced this issue May 13, 2018

Closed

Allow string to work with embedded NULs #4979

1 of 2 tasks complete

faho added a commit that referenced this issue May 28, 2018

[string] Allow `string escape` to handle NULs
TODO: This currently only works for the "script" escaping style.

Work towards #4605.

@faho faho closed this in 4dc1c6c May 28, 2018

@faho faho modified the milestones: fish-future, fish-3.0 May 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment