Everything

php

sprintf -> spprintf changes

php.internals

Timm Friebe

19 years ago

Hi,
recently, I noticed I was getting "Invalid pointer" and "4 bytes overflown"
messages executing PHP scripts connecting to sybase:

[...]php5/ext/sybase_ct/php_sybase_ct.c(818) : Block 0x09091060 status:
Invalid pointer: ((size=0x000000B6) != (next.prev=0x08410004))
Invalid pointer: ((prev=0x00000000) != (prev.size=0x000000B6))

[...]php5/Zend/zend_variables.c(175) : Block 0x09091050 status:
[...]php5/Zend/zend_execute.h(70) : Actual location (location was relayed)
Beginning:      OK (allocated on [...]php5/Zend/zend_vm_execute.h:182, 16
bytes)
Start:      OK
End:      Overflown (magic=0x0000007C instead of 0xF822F907)
At least 4 bytes overflown

Trying to track down to what this resulted from I tried the most simple form
of a test script:

$ php -r 'sybase_connect("server", "user", "password");'

...which resulted in a segmentation fault:

0x082aaefd in vspprintf (pbuf=0x1058, max_len=0, format=0x83f3ed5
"sybase_%s_%s_%s__", ap=0xbfbff5fc "À\037F\bô\037F\bh
F\blö¿¿hö¿¿´0E\b\030ú¿¿
(ú¿¿\"P-\bs F\bè`D\b\004")
at /home/thekid/devel/php/php5/main/spprintf.c:753
753             *pbuf = xbuf.c;


The fix is quite easy, spprintf wants a char**, but was given a char*. The
commits leading to the breakage read "avoid sprintf" and "simplify code"
(without further explanation).

Now I do understand that simplifying sourcecode may be a great idea, and
that in this case it really does make things easier, but I was wondering
why:

1) These changes are necessary. I don't see a bug ID in the
CVS commit log anywhere.

2) Although phpt-tests *are* provided, these most probably
weren't run after committing these changes? They would
have uncovered these problems right away.

3) While I can imagine the committers maybe don't have a sybase
server at hand to test with, and that that's the reason for
not running the tests, why don't they simply send the
maintainers, who usually have things like this at hand to test
with, a patch with the changes? Or at least send it to the
list...

4) When this new guideline of "avoiding sprintf" was introduced
(must've been a couple of weeks ago, according to the commits)
and why it was not announced - IIRC - to php-internals? Is it
some kind of secret? Should I be on IRC or at conferences to
know stuff like this?

5) If I had not - by chance - recompiled PHP this would've probably
been in 5.2.2-release, right? For code cleanliness and undocumented
anti-sprintf reasons you risk breaking existing functionality...

I thought I'd bring this to the list because maybe these kind of careless
commits have happened to other extensions too. If you're an extension autor
or maintainer, you might want to check if your source was "simplified", too.

- Timm

Ilia A.

19 years ago

On 16-Mar-07, at 4:49 AM, Timm Friebe wrote:

> 1) These changes are necessary. I don't see a bug ID in the
>   CVS commit log anywhere.

There was no specific bug the change was addressing. The goal was to
avoid potential buffer overflows inside the code possible to due
invalid length calculations for sprintf() in a good number of
instances. The change was made globally, so some safe uses, such as
the one in the sybase extension were modified as well.

> 2) Although phpt-tests *are* provided, these most probably
>   weren't run after committing these changes? They would
>   have uncovered these problems right away.
>
> 3) While I can imagine the committers maybe don't have a sybase
>   server at hand to test with, and that that's the reason for
>   not running the tests, why don't they simply send the
>   maintainers, who usually have things like this at hand to test
>   with, a patch with the changes? Or at least send it to the
>   list...

Maintainer is not always on hand and snapshots are often tested in
other environments so often a check to determine if the code is
working or not can be made based on user run tests. Unfortunately,
the sybase extension did not have anyone try it out as part of the
test suit, so this issue was not noticed until you've come across it.

> 4) When this new guideline of "avoiding sprintf" was introduced
>   (must've been a couple of weeks ago, according to the commits)
>   and why it was not announced - IIRC - to php-internals? Is it
>   some kind of secret? Should I be on IRC or at conferences to
>   know stuff like this?

The guideline is a work in progress and by the time 5.2.2 will be
released the appropriate README files will be updated.

> 5) If I had not - by chance - recompiled PHP this would've probably
>   been in 5.2.2-release, right? For code cleanliness and undocumented
>   anti-sprintf reasons you risk breaking existing functionality...

I am finalizing a sp(p)rintf() checker for PHP code right now, so it
would've detected the invalid usage, so 5.2.2 would've been safe.
That said it is a good thing you've noticed the problem and brought
it to everyone's attention.

Ilia Alshanetsky