Skip to content

remove specialized printing from phpdbg #7156

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

Merged
merged 2 commits into from
Jun 17, 2021
Merged

Conversation

krakjoe
Copy link
Member

@krakjoe krakjoe commented Jun 16, 2021

This quickly turned into a nightmare ... phpdbg is using bad format specifiers everywhere, and we got no warnings about it because they were disabled because of special format specifiers that phpdbg implemented ....

@krakjoe krakjoe force-pushed the phpdbg-printing branch 13 times, most recently from 97faa12 to f363689 Compare June 16, 2021 11:20
@krakjoe krakjoe marked this pull request as ready for review June 16, 2021 11:56
@krakjoe krakjoe requested a review from nikic June 16, 2021 11:56
@krakjoe
Copy link
Member Author

krakjoe commented Jun 16, 2021

@nikic can you take a quick glance at this please ... it turned out much more involved than I would have liked. When I enabled format checking everything blew up ... it works now, but is there anything that could be obviously better ?

@krakjoe
Copy link
Member Author

krakjoe commented Jun 16, 2021

Damn it, no vasprintf on windows and 'p' looks broken in ap_php_*

@cmb69 any ideas ?

@nikic
Copy link
Member

nikic commented Jun 16, 2021

You're looking for a _phpdbg_asprintf replacement right? This looks the same as zend_spprintf to me (with the difference that spprintf uses per-request allocator).

@krakjoe
Copy link
Member Author

krakjoe commented Jun 16, 2021

@nikic the problem seems to be that whatever api we call we end up in format_converter, which has a broken 'p' specifier (unused in php-src, I think, so uncaught). I wish I'd noticed this 8 hours ago when I started.

My last commit makes the output correct, although some segv on windows doesn't necessarily look related ... so awaiting av run.

@nikic
Copy link
Member

nikic commented Jun 16, 2021

@krakjoe I see. %pd is a custom modifier for zend_long and just %p gets eaten as a modifier, even though it's supposed to be a specifier.

We stopped using %pd a long time ago in php-src so we can enable printf format checks. I'm a bit worried that extensions may be using it, but then those extensions must have been ignoring printf format warnings for years now, so I think it's okay to drop support.

@nikic
Copy link
Member

nikic commented Jun 16, 2021

Looks like the spprintf implementation actually handles this correctly:

php-src/main/spprintf.c

Lines 342 to 349 in 591dcdb

char __next = *(fmt+1);
if ('d' == __next || 'u' == __next || 'x' == __next || 'o' == __next) {
fmt++;
modifier = LM_PHP_INT_T;
} else {
modifier = LM_STD;
}
}

@nikic
Copy link
Member

nikic commented Jun 16, 2021

#7159 for a defensive approach to removing p support.

@krakjoe
Copy link
Member Author

krakjoe commented Jun 16, 2021

Rebased on #7159, thanks ... await CI ... should be good now, I think ...

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks OK. I think we'd usually use uint32_t for linenos, but it doesn't really matter either way.

static inline void php_sapi_phpdbg_flush(void *context) /* {{{ */
{
if (!phpdbg_active_sigsafe_mem()) {
fflush(PHPDBG_G(io)[PHPDBG_STDOUT].ptr);
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering why this is gone now?

Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer write buffered (FILE*) streams anywhere, this looks like a left over relic rather than necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense.

@krakjoe krakjoe merged commit 6318040 into php:master Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants