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

src: audit and fix shift-into-sign-bit bugs #34827

Closed
bnoordhuis opened this issue Aug 18, 2020 · 4 comments
Closed

src: audit and fix shift-into-sign-bit bugs #34827

bnoordhuis opened this issue Aug 18, 2020 · 4 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors.

Comments

@bnoordhuis
Copy link
Member

$ git grep -n  '<< 24' src/
src/base64.h:88:        unbase64(src[i + 0]) << 24 |
src/cares_wrap.cc:83:  return static_cast<uint32_t>(p[0] << 24U) |
src/node_sockaddr.cc:305:  uint32_t check = ptr[0] << 24 | ptr[1] << 16 | ptr[2] << 8 | ptr[3];
src/util-inl.h:51:  (((x) & 0xFF) << 24) |                                                      \
src/util-inl.h:61:  (((x) & 0x0000000000FF0000ull) << 24) |

The LHS here is usually of type unsigned char but gets promoted to int (because 24 is an int) and is then left-shifted.

If the LHS >= 128, that ends up shifting into the sign bit and that's implementation-defined behavior (i.e., bad - although it probably works okay with most compilers.)

Replace 24 with 24u and all is good. But! It's probably best to abstract it away into a helper function.

cares_wrap.cc already has one - cares_get_32bit() - that handles it correctly.

@bnoordhuis bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. labels Aug 18, 2020
shrish-pathak pushed a commit to shrish-pathak/node that referenced this issue Aug 18, 2020
@juanarbol
Copy link
Member

Quick question, why can't cares_get_32bit() be used or considered as that helper (like here)? That function could work with

src/base64.h:88:        unbase64(src[i + 0]) << 24 |
src/cares_wrap.cc:83:  return static_cast<uint32_t>(p[0] << 24U) |
src/node_sockaddr.cc:305:  uint32_t check = ptr[0] << 24 | ptr[1] << 16 | ptr[2] << 8 | ptr[3];

@bnoordhuis
Copy link
Member Author

@juanarbol It's okay to rename and move that function to e.g. src/util.h.

@juanarbol
Copy link
Member

Ok, I'll be working on this.

@juanarbol
Copy link
Member

I don't find a way to

  inline static int8_t unbase64(uint8_t x);

  // Send all this as a const unsigned char pointer
  // Or convert all this elements into a const unsigned char pointer
  unbase64(src[i + 0]);
  unbase64(src[i + 1]);
  unbase64(src[i + 2]);
  unbase64(src[i + 3]);

  // and send it to our beautiful method
  cares_get_32bit(&mentioned_above_const_char);

juanarbol added a commit to juanarbol/node that referenced this issue Sep 9, 2020
juanarbol added a commit to juanarbol/node that referenced this issue Oct 18, 2020
BethGriggs pushed a commit that referenced this issue Dec 9, 2020
Fixes: #34827

Backport-PR-URL: #35701
PR-URL: #34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this issue Dec 10, 2020
Fixes: #34827

Backport-PR-URL: #35701
PR-URL: #34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this issue Dec 15, 2020
Fixes: #34827

Backport-PR-URL: #35701
PR-URL: #34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
Fixes: nodejs#34827

PR-URL: nodejs#34944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

2 participants