Use sprintf('%d') like in DB2, SQLServer and Oracle to harden against wrong limit and offset #4999
Conversation
src/Platforms/SqlitePlatform.php
Outdated
| @@ -765,7 +765,7 @@ protected function getPostAlterTableIndexForeignKeySQL(TableDiff $diff) | |||
| protected function doModifyLimitQuery($query, $limit, $offset) | |||
| { | |||
| if ($limit === null && $offset > 0) { | |||
| return $query . ' LIMIT -1 OFFSET ' . $offset; | |||
| return $query . sprintf(' LIMIT -1 OFFSET %d', $offset); | |||
There was a problem hiding this comment.
Should $query also be moved under sprintf()? This way, we'll have one variable / memory allocation fewer .
There was a problem hiding this comment.
Since this is in a return line it shouldn't really matter anymore.
There was a problem hiding this comment.
It still does. In order to return a value, the interpreter needs to allocate a new zval, copy the value of $query there, then append the value returned by sprintf(), then return it. If it's just sprintf(), this could be avoided.
We're of course not trying to save CPU ticks here but a combination of sprintf() and . on the same line looks dirty to me.
There was a problem hiding this comment.
Done.
What about the $query .= sprintf in all the other platforms, they are okay?
morozov
changed the title
|
Since it's not a bugfix but is more of an improvement, should this be retargeted against |
|
I can rebase on 3.2.x if you want, but since it's a hardening to a security related problem I thought it might make sense to have it there. |
|
The actual security problem is solved already, at least to my knowledge. So let's do the hardening on the next feature release. |
|
@nickvergessen please rebase on top of |
… wrong limit and offset Signed-off-by: Joas Schilling <coding@schilljs.com>
nickvergessen
force-pushed
the
sanitize-limit-parameters-like-in-oracle
branch
from
9e6d5c5
to
16de953
Compare
Rebased |
Summary
In Oracle, SQLServer and DB2 the limit and offset are handled via a
%din a sprintf() call already, adding this to the other platforms here should prevent further issues in case #4984 is ever reverted by accident again.