Skip to content
New issue

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

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

Already on GitHub? # to your account

bf_set_ui when LIMB_BITS == 32 and shift = 0 depends on undefined behavior #131

Closed
ailisp opened this issue Sep 29, 2022 · 2 comments
Closed

Comments

@ailisp
Copy link

ailisp commented Sep 29, 2022

This expression in libbf.c, function bf_set_ui, when LIMB_BITS == 32 and shift = 0:

(a0 >> (LIMB_BITS - shift))

becomes

(a0 >> 32)

And a0 is uint32_t, according to C99 and C17 spec (for bitwise shift):

If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.

So shifting it for 32 bit is an undefined behavior, and it did produce wrong result in our JS-on-Wasm runtime based on quickjs.

A quick snippet to repro:

void *realloc2(void *opaque, void *ptr, size_t size) {
  return realloc(ptr, size);
}
  bf_t a;
  bf_context_t ctx;
  bf_context_init(&ctx, realloc2, NULL);
  bf_init(&ctx, &a);
  bf_set_ui(&a, 0x8b0a00a425000000ul);
  
  uint64_t ret=0;
  bf_get_uint64(&ret, &a);

// ret is 0xaf0a00a425000000
@ailisp
Copy link
Author

ailisp commented Sep 29, 2022

Note that the above ret depends on the compiler and the compiler flags. A minimum repro to this undefined behavior:

#define LIMB_BITS 32
#include <stdio.h>
#include <stdint.h>

uint32_t opaque(uint32_t x) {
  return x == 0x3;
}

int main() {
  uint32_t temp = 7;
  uint32_t x = 0x8b0a00a4;
  uint32_t k = LIMB_BITS - opaque(x) * LIMB_BITS;
  temp = temp >> k;
  printf("%d\n", k); // 32
  printf("%d\n", temp);
  printf("%d\n", temp>>32);
}

On my system, gcc, gcc -Os, clang and clang -Oz produced different result.

ulan added a commit to ulan/javy that referenced this issue Dec 1, 2023
As reported upstream in bellard/quickjs#131:
shifting by `(LIMB_BITS - shift)` when `LIMB_BITS == 32` and `shift == 0`
produces wrong results on 32-bit targets.

This fix replaces all occurences of `(LIMB_BITS - shift)` with
calls to the existing helpers: `shld()` and `shrd`, which are
moved to the beginning of the file so that they are defined
before their new usages.

The helpers properly handle the cases when `shift` is zero.

Fixes issue 556
ulan added a commit to ulan/javy that referenced this issue Dec 1, 2023
As reported upstream in bellard/quickjs#131:
shifting by `(LIMB_BITS - shift)` when `LIMB_BITS == 32` and `shift == 0`
produces wrong results on 32-bit targets.

This fix replaces all occurences of `(LIMB_BITS - shift)` with
calls to the existing helpers: `shld()` and `shrd`, which are
moved to the beginning of the file so that they are defined
before their new usages.

The helpers properly handle the cases when `shift` is zero.

Fixes issue 556
@bellard
Copy link
Owner

bellard commented Dec 1, 2023

same as #133

@bellard bellard closed this as completed Dec 1, 2023
saghul pushed a commit to quickjs-ng/quickjs that referenced this issue Dec 2, 2023
saghul pushed a commit to quickjs-ng/quickjs that referenced this issue Dec 2, 2023
GerHobbelt pushed a commit to GerHobbelt/quickjs that referenced this issue Jun 21, 2024
Bumps [minimatch](https://github.com/isaacs/minimatch) from 3.0.4 to 3.1.2.
- [Changelog](https://github.com/isaacs/minimatch/blob/main/changelog.md)
- [Commits](isaacs/minimatch@v3.0.4...v3.1.2)

---
updated-dependencies:
- dependency-name: minimatch
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
bluesky950520 pushed a commit to bluesky950520/quickjs that referenced this issue Mar 14, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants