Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8233364

Fix undefined behavior in Canonicalizer::do_ShiftOp

    Details

    • Subcomponent:
    • Resolved In Build:
      b23

      Backports

        Description

        In do_ShiftOp, when both arguments are constants, it computes the result at compile-time, being careful to maintain Java semantics. However, there are several problems with that computation. There is a mask whose computation invokes undefined behavior.

        The code is

        jint mask = ~(~0 << (32 - shift));
        if (shift == 0) mask = ~0;

        The first problem is that the left shift of ~0 is a left shift of a negative number. C89 and C++98 don't explicitly specify the behavior, though one might infer there could be difficulties due to the different representations that are permitted. C99 and C++11 both explicitly specify this to be undefined. gcc warns about that shift when using -Wshift-negative-value (enabled by -Wextra) when compiling for C99 or C++11. This isn't a problem now, but will be for JEP 347.

        The second problem exists today. If shift == 0, which is possible and is checked for by the second line, the shift quantity is 32, which is undefined behavior in any version of C/C++ when the left hand side has a size of 32 bits (or less). This means that a sufficiently aggressive compiler might elide the second line entirely, since the test can't be true without having already invoked undefined behavior.

        There is a third problem a couple lines later with this line:

        case Bytecodes::_ishl: set_constant(value << shift); return;

        If value is negative, this is again a shift of a negative value. The C++ compiler probably isn't going to detect or be affected by this, but a runtime sanitizer could complain.

        There is a second occurrence of essentially the same code with the same problems a few lines later for the jlong case.

          Attachments

            Issue Links

              Activity

                People

                • Assignee:
                  kbarrett Kim Barrett
                  Reporter:
                  kbarrett Kim Barrett
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  5 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: