Opened 4 years ago

Closed 4 years ago

#12340 closed bug (fixed)

FlattenPictureTest kills app_server (Regression)

Reported by: jackburton Owned by: axeld
Priority: critical Milestone: Unscheduled
Component: Servers/app_server Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All


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.

Change History (5)

comment:1 Changed 4 years ago by pulkomandy

Can it be run under test_app_server for easier debugging?

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by mmlr

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.