Skip to content

Conversation

@withinboredom
Copy link
Member

The zend_ast_attr type was defined as uint16_t, limiting it to 16 bits, but it’s used to store ZEND_ACC_* flags which can use bits up to 31:

  • ZEND_ACC_OVERRIDE = (1 << 28)
  • ZEND_ACC_ENUM = (1 << 28)
  • ZEND_ACC_STRICT_TYPES = (1U << 31)

While the current code doesn’t appear to assign these high-bit flags to ast->attr fields, the type mismatch creates a potential bug where any future code attempting to store ZEND_ACC flags with bits 16–31 would have those bits silently truncated to zero.

This change:

  1. Changes zend_ast_attr typedef from uint16_t to uint32_t
  2. Adds an explicit uint16_t __pad field to all AST structures after the kind field to maintain the same memory layout (padding was already present implicitly due to alignment)

The structure sizes remain unchanged: we’re making existing implicit padding explicit and widening the attr field to properly accommodate all ZEND_ACC_* flag values.

The zend_ast_attr type was defined as uint16_t, limiting it to 16 bits,
but it's used to store ZEND_ACC_* flags which can use bits up to 31:

- ZEND_ACC_OVERRIDE = (1 << 28)
- ZEND_ACC_ENUM = (1 << 28)
- ZEND_ACC_STRICT_TYPES = (1U << 31)

While current code doesn't appear to assign these high-bit flags to
ast->attr fields, the type mismatch creates a potential bug where any
future code attempting to store ZEND_ACC flags with bits 16-31 would
have those bits silently truncated to zero.

This change:
1. Changes zend_ast_attr typedef from uint16_t to uint32_t
2. Adds explicit uint16_t __pad field to all AST structures after the
   kind field to maintain the same memory layout (padding was already
   present implicitly due to alignment)

The structure sizes remain unchanged: we're making existing implicit
padding explicit and widening the attr field to properly accommodate
all ZEND_ACC_* flag values.
Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

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

Don't add these padding fields. You could just extend the kind to 32-bits as well if you want to make it more explicit.

@iluuu1994
Copy link
Member

Is this really worthwhile? We can probably shuffle around the flags to make it fit in the 16 bit field if necessary. We also basically just ran out of 32 bit flags anyway, so some shuffling is necessary regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants