Paths

Table of Contentst

Differential D19784

Update PhabricatorLiskDAO::chunkSQL() for new %Q semantics
ClosedPublic
Actions

Authored by epriestley on Nov 7 2018, 1:06 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Wed, Dec 4, 9:08 AM

	Unknown Object (File)
	Mon, Dec 2, 11:48 PM

	Unknown Object (File)
	Mon, Dec 2, 11:48 PM

	Unknown Object (File)
	Mon, Dec 2, 11:45 AM

	Unknown Object (File)
	Mon, Nov 25, 3:02 AM

	Unknown Object (File)
	Wed, Nov 20, 5:39 PM

	Unknown Object (File)
	Sun, Nov 17, 1:37 AM

	Unknown Object (File)
	Wed, Nov 13, 4:15 AM

Subscribers

Restricted Owners Package

Details

Reviewers

Maniphest Tasks

T13217: Upgrading: Hardening of qsprintf()

Commits

rPda40f8074106: Update PhabricatorLiskDAO::chunkSQL() for new %Q semantics

Summary

Ref T13217. This method is slightly tricky:

We can't safely return a string: return an array instead.
It no longer makes sense to accept glue. All callers use ', ' as glue anyway, so hard-code that.

Then convert all callsites.

Test Plan

Browsed around, saw fewer "unsafe" errors in error log.

Diff Detail

Repository

Branch

qobject4

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 21099
Build 28676: Run Core Tests
Build 28675: arc lint + arc unit

Event Timeline

epriestley created this revision.Nov 7 2018, 1:06 AM

Owners added a subscriber: Restricted Owners Package.Nov 7 2018, 1:06 AM

Herald added a subscriber: yelirekim. · View Herald TranscriptNov 7 2018, 1:06 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 7 2018, 1:07 AM

Harbormaster failed remote builds in B21099: Diff 47250!

Tests are likely to fail since this depends on %LQ out of libphutil/ in D19783.

epriestley added a child revision: D19785: Make SELECT construction in PolicyAwareQuery safer.Nov 7 2018, 10:40 AM

amckinley accepted this revision.Nov 7 2018, 9:12 PM

amckinley added inline comments.

src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
218	Another place where a `TODO` might be appropriate.
244–247	And this is dead code because we're now doing this work inside the `%LQ` conversion, right?

This revision is now accepted and ready to land.Nov 7 2018, 9:12 PM

epriestley marked an inline comment as done.Nov 13 2018, 4:58 PM

epriestley added inline comments.

src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
244–247	Right -- `%LQ` (or some other `%L`) now do the joins safely, and `implode(...)` is "unsafe". (We could* make this "safe" and keep the semantics, but we'd have to whitelist acceptable glue, and we only ever use `,` as glue. I'm guessing third-party code has very few `chunkSQL()` calls since it's mostly an optimization / migration / workaround-for-low-mysql-packet-size thing.)

Closed by commit rPda40f8074106: Update PhabricatorLiskDAO::chunkSQL() for new %Q semantics (authored by epriestley). · Explain WhyNov 13 2018, 4:59 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

resources/

sql/

autopatches/

20140805.boardcol.2.php

2 lines

patches/

20130820.file-mailkey-populate.php

4 lines

20131106.diffphid.2.mig.php

4 lines

src/

applications/

cache/

PhabricatorKeyValueDatabaseCache.php

2 lines

calendar/

notifications/

PhabricatorCalendarNotificationEngine.php

2 lines

diffusion/

conduit/

DiffusionUpdateCoverageConduitAPIMethod.php

4 lines

diviner/

publisher/

DivinerLivePublisher.php

4 lines

fact/

daemon/

PhabricatorFactDaemon.php

2 lines

storage/

PhabricatorFactDimension.php

2 lines

maniphest/

editor/

ManiphestTransactionEditor.php

2 lines

people/

storage/

PhabricatorUserCache.php

2 lines

project/

storage/

PhabricatorProject.php

2 lines

repository/

management/

PhabricatorRepositoryManagementParentsWorkflow.php

4 lines

worker/

commitchangeparser/

PhabricatorRepositoryCommitChangeParserWorker.php

2 lines

search/

engineextension/

PhabricatorFerretFulltextEngineExtension.php

2 lines

management/

PhabricatorSearchManagementNgramsWorkflow.php

2 lines

system/

engine/

PhabricatorSystemActionEngine.php

2 lines

infrastructure/

customfield/

field/

PhabricatorCustomFieldList.php

2 lines

storage/

__tests__/

QueryFormattingTestCase.php

24 lines

lisk/

PhabricatorLiskDAO.php

17 lines

__tests__/

LiskChunkTestCase.php

32 lines

Diff 47250

resources/sql/autopatches/20140805.boardcol.2.php

Loading...

resources/sql/patches/20130820.file-mailkey-populate.php

Loading...

resources/sql/patches/20131106.diffphid.2.mig.php

Loading...

src/applications/cache/PhabricatorKeyValueDatabaseCache.php

Loading...

src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php

Loading...

src/applications/diffusion/conduit/DiffusionUpdateCoverageConduitAPIMethod.php

Loading...

src/applications/diviner/publisher/DivinerLivePublisher.php

Loading...

src/applications/fact/daemon/PhabricatorFactDaemon.php

Loading...

src/applications/fact/storage/PhabricatorFactDimension.php

Loading...

src/applications/maniphest/editor/ManiphestTransactionEditor.php

Loading...

src/applications/people/storage/PhabricatorUserCache.php

Loading...

src/applications/project/storage/PhabricatorProject.php

Loading...

src/applications/repository/management/PhabricatorRepositoryManagementParentsWorkflow.php

Loading...

src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php

Loading...

src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php

Loading...

src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php

Loading...

src/applications/system/engine/PhabricatorSystemActionEngine.php

Loading...

src/infrastructure/customfield/field/PhabricatorCustomFieldList.php

Loading...

src/infrastructure/storage/tests/QueryFormattingTestCase.php

Loading...

src/infrastructure/storage/lisk/PhabricatorLiskDAO.php

Loading...

src/infrastructure/storage/lisk/tests/LiskChunkTestCase.php

Loading...