FlattenPictureTest kills app_server (Regression)

[jackburton]

Add the FlattenPictureTest to the build. Run it. It will kill app_server. This is a regression, since it used to work correctly. Most probably related to some change in libbe<->app_server communication.

[pulkomandy]

Can it be run under test_app_server for easier debugging?

[jackburton]

I'm not sure, but I guess it's possible. Anyway, I managed to investigate it a bit, and isolated it to be tied to BPicture archiving/unarchiving. The crash is caused by a debugger() call into PicturePlayer::Play(). Changing the debugger() call to a syslog() avoids the app_server crash, but then the application (FlattenPictureTest) crashes.

[mmlr]

The code of PicturePlayer::Play() looks very dangerous in general. It uses signed integers for reading the size, but doesn't check for sizes < 0 anywhere, making it possible to go backwards in the buffer. It also does just cast the data to the needed type without checking if the size actually matches the type, which can easily lead to buffer overflows (accidental or otherwise, since the size field comes from the supplied buffer it should not be blindly trusted).

Code style wise it also uses magic numbers for the field header size everywhere instead of making it a packed struct and using sizeof().

In this concrete case the debugger is called due to bogus values for the opcode and size coming from the buffer (the values are random, so it looks like uninitialized memory is used). It is triggered when playing the unarchived picture of the no-op test, while the original picture (the one not archived and then unarchived) plays fine, indicating that there's a problem with archiving/unarchiving an empty BPicture.

I'll go ahead and rework PicturePlayer::Play() later today if noone else feels inclined to take a look.

[jackburton]

I won't have access to a development machine until tomorrow evening, but I'm not sure I'll have time, so please go ahead. Thanks.

[mmlr]

The safety issues of PicturePlayer (and the ones on the app_server, by using the new API) and the broken BPicture archive constructor are all fixed in hrev49620.

Note that unarchiving a BPicture seems to be generally broken on x86_64. I'm investigating that separately.