r96585 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r96584‎ | r96585 | r96586 >
Date:16:59, 8 September 2011
Author:nikerabbit
Status:resolved (Comments)
Tags:
Comment:
Switch JSON to (un)serialize
Modified paths:
  • /trunk/phase3/includes/logging/LogEntry.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/logging/LogEntry.php
@@ -203,13 +203,21 @@
204204 public function getParameters() {
205205 if ( !isset( $this->params ) ) {
206206 $blob = $this->getRawParameters();
207 - $params = FormatJson::decode( $blob, true /* array */ );
208 - if ( $params !== null ) {
 207+ wfSuppressWarnings();
 208+ $params = unserialize( $blob );
 209+ wfRestoreWarnings();
 210+ if ( $params !== false ) {
209211 $this->params = $params;
210212 $this->legacy = false;
211213 } else {
212 - $this->params = explode( "\n", $blob );
213 - $this->legacy = true;
 214+ $params = FormatJson::decode( $blob, true /* array */ );
 215+ if ( $params !== null ) {
 216+ $this->params = $params;
 217+ $this->legacy = false;
 218+ } else {
 219+ $this->params = explode( "\n", $blob );
 220+ $this->legacy = true;
 221+ }
214222 }
215223 }
216224 return $this->params;
@@ -319,7 +327,7 @@
320328 }
321329
322330 /**
323 - * Set extra log parameters.
 331+ * Set extra log parameters.
324332 * You can pass params to the log action message
325333 * by prefixing the keys with a number and colon.
326334 * The numbering should start with number 4, the
@@ -380,7 +388,7 @@
381389 'log_title' => $this->getTarget()->getDBkey(),
382390 'log_page' => $this->getTarget()->getArticleId(),
383391 'log_comment' => $this->getComment(),
384 - 'log_params' => FormatJson::encode( (array) $this->getParameters() ),
 392+ 'log_params' => serialize( (array) $this->getParameters() ),
385393 );
386394 $dbw->insert( 'logging', $data, __METHOD__ );
387395 $this->id = !is_null( $id ) ? $id : $dbw->insertId();
@@ -414,7 +422,7 @@
415423 $this->getSubtype(),
416424 $this->getTarget(),
417425 $this->getComment(),
418 - FormatJson::encode( (array) $this->getParameters() ),
 426+ serialize( (array) $this->getParameters() ),
419427 $newId
420428 );
421429

Comments

#Comment by Aaron Schulz (talk | contribs)   17:13, 8 September 2011

Why the switch? How easy would it be for non-PHP scripts scanning the DB tables (or dumps) to unserialize the params now?

#Comment by Nikerabbit (talk | contribs)   17:15, 8 September 2011

On wikitech it was pointed out that we already use serialized data on many database fields. I don't believe performance plays a role here.

#Comment by Aaron Schulz (talk | contribs)   17:20, 8 September 2011

Are those fields that people might be interested in for offline purposes? Are there stand-alone PHP serialization format unserialize tools that don't need PHP? I know there are a million JSON parsers in lots of languages by now.

#Comment by MaxSem (talk | contribs)   17:34, 8 September 2011

Join the ML discussion.

#Comment by Aaron Schulz (talk | contribs)   06:17, 9 September 2011

Does this really need b/c for JSON just to support a few revs back in /trunk?

#Comment by Nikerabbit (talk | contribs)   06:19, 9 September 2011

Not really, it is only me who has few of those. But who knows if the default is still going to be switched few times :)

#Comment by DaSch (talk | contribs)   19:00, 11 September 2011

this changes seams to cause the following bug https://bugzilla.wikimedia.org/show_bug.cgi?id=30854

#Comment by Bawolff (talk | contribs)   19:35, 16 September 2011

Is there any security considerations for using unserialize like this? Its not unconcievable that the log_params for some log types could be entirely user supplied (although off hand I don't know if there are any logs that that is true for). I know that unserializing objects will call __wakeup() on them (that requires an already existing class with a __wakeup method that does something evil though). I don't really know if we have any classes where you can do evil stuff with that (or if their is other more applicable ways of doing evil with serialized objects), but it sounds kind of sketchy to test if the log_params are a serialized array by simply unserializing it.

#Comment by Nikerabbit (talk | contribs)   19:58, 16 September 2011

I don't think there is any, and the hole will soon close up when logs are converted to produce new style param storage.

#Comment by Aaron Schulz (talk | contribs)   21:00, 16 September 2011

Moar details? :)

#Comment by Nikerabbit (talk | contribs)   13:29, 17 September 2011

About? The new code stores array of parameters as serialized blob.

Status & tagging log