Skip to content

Fix a few issues which can lead to undefined-behaviour #1730

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 13, 2017
Merged

Fix a few issues which can lead to undefined-behaviour #1730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jerry-core/ecma/base/ecma-helpers-value.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ ecma_make_integer_value (ecma_integer_value_t integer_value) /**< integer number
{
JERRY_ASSERT (ECMA_IS_INTEGER_NUMBER (integer_value));

return ((ecma_value_t) (integer_value << ECMA_DIRECT_SHIFT)) | ECMA_DIRECT_TYPE_INTEGER_VALUE;
return (((ecma_value_t) integer_value) << ECMA_DIRECT_SHIFT) | ECMA_DIRECT_TYPE_INTEGER_VALUE;
} /* ecma_make_integer_value */

/**
Expand Down
6 changes: 4 additions & 2 deletions jerry-core/parser/regexp/re-bytecode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ re_bytecode_list_insert (re_bytecode_ctx_t *bc_ctx_p, /**< RegExp bytecode conte
inline ecma_char_t __attr_always_inline___
re_get_char (uint8_t **bc_p) /**< pointer to bytecode start */
{
ecma_char_t chr = *((ecma_char_t *) *bc_p);
ecma_char_t chr;
memcpy (&chr, *bc_p, sizeof (ecma_char_t));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly was undefined behaviour in the original code? Is memcpy the only way to get around it? As it is a call to a libc function, it is much more expensive than a simple cast.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading uint16 from byte aligned address is undefined in C.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that. But cannot we have a macro that does the uint16 read from a byte pointer "in place" without the function call overhead (and the loop overhead of memcpy)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any answer to this, even though it got merged already?...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know most compilers are proud to optimize this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't trust them we could create a macro. Problem is, if the architecture supports unaligned read, we are expecting the compiler to deoptimize our macro.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw GCC has a flag to not optimize this: -fno-builtin-memcpy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that we have our own memcpy implementation in jerry-libc and I'm not sure that gcc can optimize it in that case. Especially as we are compiling jerry with -fno-builtin, which has effect on memcpy as well.

(*bc_p) += sizeof (ecma_char_t);
return chr;
} /* re_get_char */
Expand All @@ -152,7 +153,8 @@ re_get_opcode (uint8_t **bc_p) /**< pointer to bytecode start */
inline uint32_t __attr_always_inline___
re_get_value (uint8_t **bc_p) /**< pointer to bytecode start */
{
uint32_t value = *((uint32_t *) *bc_p);
uint32_t value;
memcpy (&value, *bc_p, sizeof (uint32_t));
(*bc_p) += sizeof (uint32_t);
return value;
} /* re_get_value */
Expand Down
3 changes: 2 additions & 1 deletion jerry-core/vm/vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2754,7 +2754,8 @@ vm_run (const ecma_compiled_code_t *bytecode_header_p, /**< byte-code data heade
frame_ctx.is_eval_code = is_eval_code;
frame_ctx.call_operation = VM_NO_EXEC_OP;

ecma_value_t stack[call_stack_size];
/* Use JERRY_MAX() to avoid array declaration with size 0. */
ecma_value_t stack[JERRY_MAX (call_stack_size, 1)];
frame_ctx.registers_p = stack;

return vm_execute (&frame_ctx, arg_list_p, arg_list_len);
Expand Down