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

Built-in `printf` truncates integers to 32-bit #3352

Closed
aburgh opened this Issue Sep 6, 2016 · 6 comments

Comments

Projects
None yet
5 participants
@aburgh

aburgh commented Sep 6, 2016

  • Have you checked if problem occurs with fish 2.3.1?
  • Tried fish without third-party customizations (check sh -c 'env HOME=$(mktemp -d) fish')?

fish version installed (fish --version):
2.3.1

OS/terminal used:
macOS, Terminal

Integers to the built-in printf are truncated to 32 bit.

Reproduction steps

  1. Enter printf "%#x\\n" 498216206596

Expected results

> command printf "%#x\\n" 498216206596
0x7400000104

Actual results

> printf  "%#x\\n" 498216206596
0x104
@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 6, 2016

Member

Indeed. Should output 0x7400000101

raw_string_to_scalar_type in builtin_printf.cpp has some shenanigans going on with the types there might be it.

Member

floam commented Sep 6, 2016

Indeed. Should output 0x7400000101

raw_string_to_scalar_type in builtin_printf.cpp has some shenanigans going on with the types there might be it.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 6, 2016

Member

FWIW it'll still occur if one fixes up to the templates to be:

template <>
intmax_t raw_string_to_scalar_type(const wchar_t *s, wchar_t **end) {
    return wcstoimax(s, end, 0);
}

template <>
uintmax_t raw_string_to_scalar_type(const wchar_t *s, wchar_t **end) {
    return wcstoumax(s, end, 0);
}

(there was a change to accommodate FreeBSD 8: #626 - changing the types back is still the right thing to do, I think. FreeBSD seems to have fixed their headers.)

Member

floam commented Sep 6, 2016

FWIW it'll still occur if one fixes up to the templates to be:

template <>
intmax_t raw_string_to_scalar_type(const wchar_t *s, wchar_t **end) {
    return wcstoimax(s, end, 0);
}

template <>
uintmax_t raw_string_to_scalar_type(const wchar_t *s, wchar_t **end) {
    return wcstoumax(s, end, 0);
}

(there was a change to accommodate FreeBSD 8: #626 - changing the types back is still the right thing to do, I think. FreeBSD seems to have fixed their headers.)

@floam floam self-assigned this Sep 6, 2016

@faho faho added the bug label Sep 6, 2016

@faho faho added this to the next-2.x milestone Sep 6, 2016

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 13, 2016

Contributor

The problem is that we're not using int64_t and uint64_t types for ints. The GNU printf does and furthermore warns when the value is too large:

$ command printf "%#x\\n" 18446744073709551614
0xfffffffffffffffe
$ command printf "%#x\\n" 18446744073709551615
0xffffffffffffffff
$ command printf "%#x\\n" 18446744073709551616
printf: ‘18446744073709551616’: Result too large
0xffffffffffffffff

To be compatible we should be using wcstoll() and wcstoull() where we currently use wcstoimax() and wcstoumax.

Contributor

krader1961 commented Nov 13, 2016

The problem is that we're not using int64_t and uint64_t types for ints. The GNU printf does and furthermore warns when the value is too large:

$ command printf "%#x\\n" 18446744073709551614
0xfffffffffffffffe
$ command printf "%#x\\n" 18446744073709551615
0xffffffffffffffff
$ command printf "%#x\\n" 18446744073709551616
printf: ‘18446744073709551616’: Result too large
0xffffffffffffffff

To be compatible we should be using wcstoll() and wcstoull() where we currently use wcstoimax() and wcstoumax.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Nov 16, 2016

Member

Heh, does GNU printf do this when compiled in 32 bit mode? (Answer: yes!)

I don't understand the suggestion to switch from wcstoumax to wcstoull. Isn't uintmax_t just unsigned long long anyways?

Member

ridiculousfish commented Nov 16, 2016

Heh, does GNU printf do this when compiled in 32 bit mode? (Answer: yes!)

I don't understand the suggestion to switch from wcstoumax to wcstoull. Isn't uintmax_t just unsigned long long anyways?

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Nov 16, 2016

Member

It looks to me like the problem is that we are constructing a format string (in print_direc) that doesn't specify the field width for '%x' conversions.

Member

ridiculousfish commented Nov 16, 2016

It looks to me like the problem is that we are constructing a format string (in print_direc) that doesn't specify the field width for '%x' conversions.

@krader1961 krader1961 modified the milestones: next-minor, fish 2.5.0 Jan 2, 2017

@krader1961 krader1961 assigned krader1961 and unassigned floam Feb 20, 2017

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Feb 20, 2017

Contributor

It turns out this is a very prosaic bug. As @ridiculousfish guessed the core problem is that the builtin_printf_state_t::print_direc() method did not include x and X in the format specifiers that need an explicit ll modifier. Fix forthcoming.

Having said that I am still very unhappy about the assumption that uintmax_t is equivalent to unsigned long long since that is not guaranteed. Fish could, theoretically, run on a system where uintmax_t is only 32-bits in length rather than 64-bits as implied by long long.

Contributor

krader1961 commented Feb 20, 2017

It turns out this is a very prosaic bug. As @ridiculousfish guessed the core problem is that the builtin_printf_state_t::print_direc() method did not include x and X in the format specifiers that need an explicit ll modifier. Fix forthcoming.

Having said that I am still very unhappy about the assumption that uintmax_t is equivalent to unsigned long long since that is not guaranteed. Fish could, theoretically, run on a system where uintmax_t is only 32-bits in length rather than 64-bits as implied by long long.

@faho faho added the release notes label Feb 25, 2017

develop7 added a commit to develop7/fish-shell that referenced this issue Apr 17, 2017

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