r97328 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r97327‎ | r97328 | r97329 >
Date:20:08, 16 September 2011
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Noticed in apache error logs (see below). If the length of the message is longer than the maximum length, only send what will fit

Sep 16 20:07:28 10.0.2.193 apache2[28441]: PHP Warning: socket_sendto() [<a href='function.socket-sendto'>function.socket-sendto</a>]: unable to write to socket [90]: Message too long in /home/wikipedia/common/php-1.17-test/includes/GlobalFunctions.php on line 464
Sep 16 20:07:29 10.0.2.193 apache2[26511]: PHP Warning: socket_sendto() [<a href='function.socket-sendto'>function.socket-sendto</a>]: unable to write to socket [90]: Message too long in /home/wikipedia/common/php-1.17-test/includes/GlobalFunctions.php on line 464
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/GlobalFunctions.php
@@ -867,7 +867,14 @@
868868 if ( !$sock ) {
869869 return;
870870 }
871 - socket_sendto( $sock, $text, strlen( $text ), 0, $host, $port );
 871+
 872+ $len = strlen( $text );
 873+ $maxLen = socket_get_option( $sock, SOL_UDP, SO_SNDBUF );
 874+
 875+ if ( $len > $maxLen ) {
 876+ $len = $maxLen - 1;
 877+ }
 878+ socket_sendto( $sock, $text, $len, 0, $host, $port );
872879 socket_close( $sock );
873880 } else {
874881 wfSuppressWarnings();

Follow-up revisions

RevisionCommit summaryAuthorDate
r97627Fix for r97328: use SOL_SOCKET instead of SOL_UDP; fixes E_WARNINGialex13:57, 20 September 2011
r97919Mostly revert r97328...reedy14:14, 23 September 2011

Comments

#Comment by Krinkle (talk | contribs)   21:51, 18 September 2011

Just taking note here of the IRC convo. I was wondering whether socket_sendto limits in number of characters or bytes. Assuming the latter we should probably use mb_strlen here, right ?

#Comment by Catrope (talk | contribs)   07:16, 19 September 2011

Dude, thank you. I've been looking at these warnings for a while but have been too lazy to fix them.

#Comment by IAlex (talk | contribs)   13:39, 20 September 2011

Now I'm getting

Warning: socket_get_option() [function.socket-get-option]: unable to retrieve socket option [42]: Protocol not available in includes/GlobalFunctions.php on line 872
Warning: socket_sendto() [function.socket-sendto]: unable to write to socket [22]: Invalid argument in includes/GlobalFunctions.php on line 877

Replacing SOL_UDP by SOL_SOCKET in socket_get_option() fixes the problem.

#Comment by Reedy (talk | contribs)   13:48, 20 September 2011

It was untested, so feel free to change that if it fixes it for you

#Comment by Tim Starling (talk | contribs)   04:09, 22 September 2011

This is wrong. SO_SNDBUF is 124KB by default, which is much larger than the maximum UDP packet size of 64KB.

> print socket_get_option( $sock, SOL_SOCKET, SO_SNDBUF )
126976

> $l = 65507; socket_sendto($sock, str_repeat('x',$l), $l, 0, '127.0.0.1', 1111);

> $l = 65508; socket_sendto($sock, str_repeat('x',$l), $l, 0, '127.0.0.1', 1111);
PHP Warning:  socket_sendto(): unable to write to socket [90]: Message too long in /home/tstarling/src/mediawiki/trunk/phase3/maintenance/eval.php(79) : eval()'d code on line 1
#Comment by Reedy (talk | contribs)   10:51, 22 September 2011

Looking at the PHP documentation for this [1]

SO_SNDBUF is "Reports the size of the send buffer.", which seemingly is what we want.

Reading it and the documentation would lead you to believe SOL_UDP is valid, but does a poor job of listing other correct values

The constants [2] show SOL_DUP along with a huge host of other values. SOCK_DGRAM would be correct for UDP, but pushing that as the middle parameter to socket_get_option() gives "unable to retrieve socket option [95]"

It could be hardcoded at 64KB (65,536) using the length in bytes per Krinkle above (rather thant he string length)... It just seems like there should be a better way to do it

Having said that, I've just noticed above

		// Clean it up for the multiplexer
		if ( strval( $prefix ) !== '' ) {
			$text = preg_replace( '/^/m', $prefix . ' ', $text );

			// Limit to 64KB
			if ( strlen( $text ) > 65534 ) {
				$text = substr( $text, 0, 65534 );
			}

			if ( substr( $text, -1 ) != "\n" ) {
				$text .= "\n";
			}
		} elseif ( strlen( $text ) > 65535 ) {
			$text = substr( $text, 0, 65535 );
		}

In theory, the larger packet condition shouldn't occur, bar, that is using strlen also, not string byte length..

Removing the code I added, and then just changing both to do the length by the byte length..

[1] http://php.net/manual/en/function.socket-get-option.php [2] http://www.php.net/manual/en/sockets.constants.php

#Comment by Tim Starling (talk | contribs)   22:46, 22 September 2011

The PHP manual is not the best place to look for documentation. socket_get_option() is just a wrapper for getsockopt(). PHP's description of it is just a summary of the underlying platform documentation, for example the relevant Linux man pages. The socket options that go with SOL_SOCKET are documented on socket(7), and the socket options that go with UDP are documented on udp(7).

It's not especially helpful in this case, but it seems likely that the size of the send buffer would indeed limit the size of UDP packets if it were set to something less than the maximum packet size of 65507 bytes. This is not the case by default, but if it did happen, you could increase the size of the send buffer using socket_set_option() to allow the sending of full-sized packets.

The PHP manual does not say that SOL_UDP exists, it says that only SOL_SOCKET exists, and that for any other protocols, you need to use the protocol number from getprotobyname().

Limiting packet size to 65535 bytes would not be correct, since the maximum UDP packet size is 65507 bytes, so any log entries between 65508 and 65535 bytes would still fail. See e.g. w:User Datagram Protocol#Packet structure.

Krinkle has got it backwards: strlen() gives the size of the string in bytes, and mb_strlen() gives the size of the string in characters. strlen() is what you want.

#Comment by Reedy (talk | contribs)   23:24, 22 September 2011

SOL_UDP is listed as being defined at Predefined Constants as "SOL_UDP (integer)", it is used further up and apparently a value of 17, which is the same as:

> print getprotobyname( 'udp' );
17

> print defined( 'SOL_UDP' );
1

Using that in socket_get_options() complains "Warning: socket_get_option(): unable to retrieve socket option [92]:"

Reading the UDP documentation, there is seemingly no option to get this information.

Would it be simplest to back this change out, and then use 65507 (and 65506 for the \n appended version) on the substring commands above to limit the length of the string?

#Comment by Tim Starling (talk | contribs)   01:38, 23 September 2011

Yes, just use 65507 (minus one). I don't know of any way to get that number from configuration. In theory you could use min( 65507, socket_get_option( $sock, SOL_SOCKET, SO_SNDBUF ) ) but it may break on OSs other than Linux, so I don't think it's worth doing.

Another option is to suppress warnings while calling socket_sendto() and then to use socket_last_error().

Status & tagging log