Skip to content
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

Compile warnings in encode_salt with GCC 7.2 #568

Closed
fgm opened this issue Jan 11, 2018 · 3 comments
Closed

Compile warnings in encode_salt with GCC 7.2 #568

fgm opened this issue Jan 11, 2018 · 3 comments

Comments

@fgm
Copy link

fgm commented Jan 11, 2018

  • What went wrong: While installing, compilation produces the warnings below
  • What did you expect to happen? No warning, as used to be the case
  • Which version of nodejs and OS?
    • node 8.9.4
    • OS: Linux 4.13.0-25-generic Random invalid salt #29-Ubuntu SMP Mon Jan 8 21:14:41 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
    • GCC version: cc (Ubuntu 7.2.0-8ubuntu3) 7.2.0
../src/bcrypt.cc: In function ‘void encode_salt(char*, u_int8_t*, u_int16_t, u_int8_t)’:
../src/bcrypt.cc:132:1: warning: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Wformat-truncation=]
 encode_salt(char *salt, u_int8_t *csalt, u_int16_t clen, u_int8_t logr)
 ^~~~~~~~~~~
In file included from /usr/include/stdio.h:862:0,
                 from ../src/bcrypt.cc:48:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:65:44: note: ‘__builtin___snprintf_chk’ output between 4 and 5 bytes into a destination of size 4
        __bos (__s), __fmt, __va_arg_pack ());
                                            ^
../src/bcrypt.cc: In function ‘void bcrypt(const char*, const char*, char*)’:
../src/bcrypt.cc:165:1: warning: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Wformat-truncation=]
 bcrypt(const char *key, const char *salt, char *encrypted)
 ^~~~~~
In file included from /usr/include/stdio.h:862:0,
                 from ../src/bcrypt.cc:48:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:65:44: note: ‘__builtin___snprintf_chk’ output between 4 and 5 bytes into a destination of size 4
        __bos (__s), __fmt, __va_arg_pack ());

Why it happens (AIUI)

  • The code invokes snprintf(encrypted + i, 4, "%2.2u$", logr); in both encode_salt() and bcrypt().
  • In both cases, the caller knows that logr <= 31, so the result will look like "xy" so will actually hold in 2 bytes.
  • However, the compiler only knows that logr is a byte, so can be anything <= 255, hence potentially 3 bytes instead of two, triggering the snprintf() truncation, so it emits the format_truncation warning, which it apparently did not in GCC 6 and lower.
  • Judging from github search results, multiple packages have been hit by this warning

Fix ideas

  • Use a wider format, like "%3.3u$". Changes expectations
  • Disable this warning, maybe using CXXFLAGS="-Wformat-truncation=0", or configuring options in binding.gyp
  • Make the compiler aware of the fact that logr is less than 0x20, maybe by invoking the function like:
snprintf(salt + 4, 4, "%2.2u$", logr & 0x20);
@recrsn
Copy link
Collaborator

recrsn commented Jan 13, 2018

Would you mind testing and making a PR perhaps?
I think anding 0x20 should be fine.

@fgm
Copy link
Author

fgm commented Jan 13, 2018

Oops, just realized it's not 0x20 (which would drop anything but bit 5), but 0x001F (to keep bits 0 to 5).

fgm added a commit to fgm/filog that referenced this issue Jan 25, 2018
- Needed because of kelektiv/node.bcrypt.js#568
- Cleant up the .eslintrc globals
- Fixed typo in comments
@recrsn
Copy link
Collaborator

recrsn commented Feb 14, 2019

Fixed since v3.0.4

@recrsn recrsn closed this as completed Feb 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants