@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Allow transaction publishers to pass binary data to workers

Summary:
Ref T8672. Ref T9187. Root issue in at least one case is:

- User makes a commit including a file with some non-UTF8 text (say, a Japanese file full of Shift-JIS).
- We pass the file to the TransactionEditor so it can inline or attach the patch if the server is configured for these things.
- When inlining patches, we convert them to UTF8 before inlining. We must do this since the rest of the mail is UTF8.
- When attaching patches, we send them in the original encoding (as file attachments). This is correct, and means we need to give the worker the raw patch in whatever encoding it was originally in: we can't just convert it to utf8 earlier, or we'd attach the wrong patch in some cases.
- TransactionEditor does its thing (e.g., creates the commit), then gets ready to send mail about whatever it did.
- The publishing work now happens in the daemon queue, so we prepare to queue a PublishWorker and pass it the patch (with some other data).
- When we queue workers, we serialize the state data with JSON.

So far, so good. But this is where things go wrong:

- JSON can't encode binary data, and can't encode Shift-JIS. The encoding silently fails and we ignore it.

Then we get to the worker, and things go wrong-er:

- Since the data is bad, we fatal. This isn't a permanent failure, so we continue retrying the task indefinitely.

This applies several fixes:

# When queueing tasks, fail loudly when JSON encoding fails.
# In the worker, fail permanently when data can't be decoded.
# Allow Editors to specify that some of their data is binary and needs special handling.

This is fairly messy, but some simpler alternatives don't seem like good ways forward:

- We can't convert to UTF8 earlier, because we need the original raw patch when adding it as an attachment.
- We could encode //only// this field, but I suspect some other fields will also need attention, so that adding a mechanism will be worthwhile. In particular, I suspect filenames //may// be causing a similar problem in some cases.
- We could convert task data to always use a serialize()-based binary safe encoding, but this is a larger change and I think it's correct that things are UTF8 by default, even if it makes a bit of a mess. I'd rather have an explicit mess like this than a lot of binary data floating around.

The change to make `LiskDAO` will almost certainly catch some other problems too, so I'm going to hold this until after `stable` is cut. These problems were existing problems (i.e., the code was previously breaking or destroying data) so it's definitely correct to catch them, but this will make the problems much more obvious/urgent than they previously were.

Test Plan:
- Created a commit with a bunch of Shift-JIS stuff in a file.
- Tried to import it.

Prior to patch:

- Broken PublishWorker with distant, irrelevant error message.

With patch partially applied (only new error checking):

- Explicit, local error message about bad key in serialized data.

With patch fully applied:

- Import went fine and mail generated.

Reviewers: chad

Reviewed By: chad

Subscribers: devurandom, nevogd

Maniphest Tasks: T8672, T9187

Differential Revision: https://secure.phabricator.com/D13939

+131 -12
+6
src/applications/audit/editor/PhabricatorAuditEditor.php
··· 949 949 ); 950 950 } 951 951 952 + protected function getCustomWorkerStateEncoding() { 953 + return array( 954 + 'rawPatch' => self::STORAGE_ENCODING_BINARY, 955 + ); 956 + } 957 + 952 958 protected function loadCustomWorkerState(array $state) { 953 959 $this->rawPatch = idx($state, 'rawPatch'); 954 960 $this->affectedFiles = idx($state, 'affectedFiles');
+118 -10
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
··· 69 69 private $feedNotifyPHIDs = array(); 70 70 private $feedRelatedPHIDs = array(); 71 71 72 + const STORAGE_ENCODING_BINARY = 'binary'; 73 + 72 74 /** 73 75 * Get the class name for the application this editor is a part of. 74 76 * ··· 2637 2639 } 2638 2640 2639 2641 2642 + /** 2643 + * @task mail 2644 + */ 2645 + private function runHeraldMailRules(array $messages) { 2646 + foreach ($messages as $message) { 2647 + $engine = new HeraldEngine(); 2648 + $adapter = id(new PhabricatorMailOutboundMailHeraldAdapter()) 2649 + ->setObject($message); 2650 + 2651 + $rules = $engine->loadRulesForAdapter($adapter); 2652 + $effects = $engine->applyRules($rules, $adapter); 2653 + $engine->applyEffects($effects, $adapter, $rules); 2654 + } 2655 + } 2656 + 2640 2657 2641 2658 /* -( Publishing Feed Stories )-------------------------------------------- */ 2642 2659 ··· 3060 3077 $state[$property] = $this->$property; 3061 3078 } 3062 3079 3080 + $custom_state = $this->getCustomWorkerState(); 3081 + $custom_encoding = $this->getCustomWorkerStateEncoding(); 3082 + 3063 3083 $state += array( 3064 3084 'excludeMailRecipientPHIDs' => $this->getExcludeMailRecipientPHIDs(), 3065 - 'custom' => $this->getCustomWorkerState(), 3085 + 'custom' => $this->encodeStateForStorage($custom_state, $custom_encoding), 3086 + 'custom.encoding' => $custom_encoding, 3066 3087 ); 3067 3088 3068 3089 return $state; ··· 3081 3102 3082 3103 3083 3104 /** 3105 + * Hook; return storage encoding for custom properties which need to be 3106 + * passed to workers. 3107 + * 3108 + * This primarily allows binary data to be passed to workers and survive 3109 + * JSON encoding. 3110 + * 3111 + * @return dict<string, string> Property encodings. 3112 + * @task workers 3113 + */ 3114 + protected function getCustomWorkerStateEncoding() { 3115 + return array(); 3116 + } 3117 + 3118 + 3119 + /** 3084 3120 * Load editor state using a dictionary emitted by @{method:getWorkerState}. 3085 3121 * 3086 3122 * This method is used to load state when running worker operations. ··· 3097 3133 $exclude = idx($state, 'excludeMailRecipientPHIDs', array()); 3098 3134 $this->setExcludeMailRecipientPHIDs($exclude); 3099 3135 3100 - $custom = idx($state, 'custom', array()); 3136 + $custom_state = idx($state, 'custom', array()); 3137 + $custom_encodings = idx($state, 'custom.encoding', array()); 3138 + $custom = $this->decodeStateFromStorage($custom_state, $custom_encodings); 3139 + 3101 3140 $this->loadCustomWorkerState($custom); 3102 3141 3103 3142 return $this; ··· 3143 3182 ); 3144 3183 } 3145 3184 3146 - private function runHeraldMailRules(array $messages) { 3147 - foreach ($messages as $message) { 3148 - $engine = new HeraldEngine(); 3149 - $adapter = id(new PhabricatorMailOutboundMailHeraldAdapter()) 3150 - ->setObject($message); 3185 + /** 3186 + * Apply encodings prior to storage. 3187 + * 3188 + * See @{method:getCustomWorkerStateEncoding}. 3189 + * 3190 + * @param map<string, wild> Map of values to encode. 3191 + * @param map<string, string> Map of encodings to apply. 3192 + * @return map<string, wild> Map of encoded values. 3193 + * @task workers 3194 + */ 3195 + final private function encodeStateForStorage( 3196 + array $state, 3197 + array $encodings) { 3198 + 3199 + foreach ($state as $key => $value) { 3200 + $encoding = idx($encodings, $key); 3201 + switch ($encoding) { 3202 + case self::STORAGE_ENCODING_BINARY: 3203 + // The mechanics of this encoding (serialize + base64) are a little 3204 + // awkward, but it allows us encode arrays and still be JSON-safe 3205 + // with binary data. 3206 + 3207 + $value = @serialize($value); 3208 + if ($value === false) { 3209 + throw new Exception( 3210 + pht( 3211 + 'Failed to serialize() value for key "%s".', 3212 + $key)); 3213 + } 3214 + 3215 + $value = base64_encode($value); 3216 + if ($value === false) { 3217 + throw new Exception( 3218 + pht( 3219 + 'Failed to base64 encode value for key "%s".', 3220 + $key)); 3221 + } 3222 + break; 3223 + } 3224 + $state[$key] = $value; 3225 + } 3226 + 3227 + return $state; 3228 + } 3229 + 3230 + 3231 + /** 3232 + * Undo storage encoding applied when storing state. 3233 + * 3234 + * See @{method:getCustomWorkerStateEncoding}. 3235 + * 3236 + * @param map<string, wild> Map of encoded values. 3237 + * @param map<string, string> Map of encodings. 3238 + * @return map<string, wild> Map of decoded values. 3239 + * @task workers 3240 + */ 3241 + final private function decodeStateFromStorage( 3242 + array $state, 3243 + array $encodings) { 3244 + 3245 + foreach ($state as $key => $value) { 3246 + $encoding = idx($encodings, $key); 3247 + switch ($encoding) { 3248 + case self::STORAGE_ENCODING_BINARY: 3249 + $value = base64_decode($value); 3250 + if ($value === false) { 3251 + throw new Exception( 3252 + pht( 3253 + 'Failed to base64_decode() value for key "%s".', 3254 + $key)); 3255 + } 3151 3256 3152 - $rules = $engine->loadRulesForAdapter($adapter); 3153 - $effects = $engine->applyRules($rules, $adapter); 3154 - $engine->applyEffects($effects, $adapter, $rules); 3257 + $value = unserialize($value); 3258 + break; 3259 + } 3260 + $state[$key] = $value; 3155 3261 } 3262 + 3263 + return $state; 3156 3264 } 3157 3265 3158 3266 }
+6 -1
src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php
··· 26 26 * Load the object the transactions affect. 27 27 */ 28 28 private function loadObject() { 29 + $viewer = PhabricatorUser::getOmnipotentUser(); 30 + 29 31 $data = $this->getTaskData(); 30 - $viewer = PhabricatorUser::getOmnipotentUser(); 32 + if (!is_array($data)) { 33 + throw new PhabricatorWorkerPermanentFailureException( 34 + pht('Task has invalid task data.')); 35 + } 31 36 32 37 $phid = idx($data, 'objectPHID'); 33 38 if (!$phid) {
+1 -1
src/infrastructure/storage/lisk/LiskDAO.php
··· 1651 1651 if ($deserialize) { 1652 1652 $data[$col] = json_decode($data[$col], true); 1653 1653 } else { 1654 - $data[$col] = json_encode($data[$col]); 1654 + $data[$col] = phutil_json_encode($data[$col]); 1655 1655 } 1656 1656 break; 1657 1657 default: