Skip to content

ext/readline: fix global readline vars when updating rl_line_buffer. #15120

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

Closed
wants to merge 2 commits into from

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Jul 27, 2024

if the new value was larger, rl_line_buffer_length was never updated. neither was rl_end (on unixes).

if the new value was larger, rl_line_buffer_length was never updated.
neither was rl_end (on unixes).
@devnexen devnexen force-pushed the readline_info_upd branch from e3cde84 to 4335e31 Compare July 27, 2024 09:01
@devnexen devnexen marked this pull request as ready for review July 27, 2024 09:25
@devnexen devnexen requested a review from Girgias July 27, 2024 09:25
if (strlen(oldstr) < Z_STRLEN_P(value)) {
rl_extend_line_buffer(Z_STRLEN_P(value) + 1);
}
strcpy(rl_line_buffer, Z_STRVAL_P(value));
Copy link
Member

Choose a reason for hiding this comment

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

Why not memcpy? We already checked that the buffer is large enough

Copy link
Member Author

Choose a reason for hiding this comment

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

that s the thing, since we know it s large enough strcpy fits by its simplicity :)

Copy link
Member Author

Choose a reason for hiding this comment

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

after 2nd thoughts, will give it a try.

@devnexen devnexen force-pushed the readline_info_upd branch from fe185fb to f472621 Compare July 27, 2024 12:40
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Looks reasonable

@devnexen devnexen closed this in a7d856d Jul 28, 2024
@cmb69
Copy link
Member

cmb69 commented Jul 30, 2024

@remicollet just reported that this broke libedit builds. Maybe somebody wants to have a look at this. And it might be a good idea to run at least one CI against libedit-dev instead of libreadline-dev.

@devnexen
Copy link
Member Author

@remicollet just reported that this broke libedit builds. Maybe somebody wants to have a look at this.

PR here

devnexen added a commit to devnexen/php-src that referenced this pull request Aug 4, 2024
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