Skip to content

/ node

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

Jump to bottom

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

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

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

bnoordhuis opened this issue Aug 18, 2020 · 4 comments
Labels
C++ good first issue

Comments

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Aug 18, 2020 

$ 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 mentioned this issue Aug 18, 2020
Open
4 of 4 tasks complete
shrish-pathak pushed a commit to shrish-pathak/node that referenced this issue Aug 18, 2020
_shrish_
Replace 24 with 24U, git grep -n '<< 24' src/, nodejs#34827
3691a46
@juanarbol
Copy link
Member

@juanarbol juanarbol commented Aug 24, 2020

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

@bnoordhuis bnoordhuis commented Aug 24, 2020

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

@juanarbol juanarbol commented Aug 24, 2020

Ok, I'll be working on this.
@juanarbol
Copy link
Member

@juanarbol juanarbol commented Aug 25, 2020

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 juanarbol mentioned this issue Aug 27, 2020
Open
2 of 2 tasks complete
juanarbol added a commit to juanarbol/node that referenced this issue Sep 9, 2020
@juanarbol
src: create helper for reading Uint32BE
d78c1be 
Fixes: nodejs#34827
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C++ good first issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@bnoordhuis @juanarbol
You can’t perform that action at this time.