| Summary: | Segfault from getSupportedWriteSpeedsVia2A | ||
|---|---|---|---|
| Product: | [Applications] k3b | Reporter: | Mark <phobian> |
| Component: | Burning/Hardware | Assignee: | k3b developers <k3b> |
| Status: | RESOLVED WORKSFORME | ||
| Severity: | crash | CC: | asturm, bugseforuns, michalm, scdbackup, simonandric5, trueg, zhaixiang, zzam |
| Priority: | NOR | ||
| Version: | 17.04.3 | ||
| Target Milestone: | --- | ||
| Platform: | Compiled Sources | ||
| OS: | Linux | ||
| Latest Commit: | https://commits.kde.org/k3b/7f0be6a33b8260f7789c6aeed58be8d1c844229a | Version Fixed In: | |
| Sentry Crash Report: | |||
| Attachments: | Out from k3b and cdrecord -prcap | ||
Hi Mark,
Thanks for your report!
> if( !data.size() > 32) {
it doesn't look good to me, please debug gdb, when segfault, just bt to find the root cause, thanks!
Regards,
Leslie Zhai
Hi Leslie,
Hope it helps:
Thread 1 "k3b" received signal SIGSEGV, Segmentation fault.
0x00007ffff67148e8 in K3b::Device::from2Byte (d=0x7ffffffff002 <error: Cannot access memory at address 0x7ffffffff002>)
at /home/mark/Sources/k3b/libk3bdevice/k3bdeviceglobals.cpp:221
221 return ( (d[0] << 8 & 0xFF00) |
(gdb) bt
#0 0x00007ffff67148e8 in K3b::Device::from2Byte(unsigned char const*) (d=0x7ffffffff002 <error: Cannot access memory at address 0x7ffffffff002>) at /home/mark/Sources/k3b/libk3bdevice/k3bdeviceglobals.cpp:221
#1 0x00007ffff6700956 in K3b::Device::Device::getSupportedWriteSpeedsVia2A(QList<int>&, K3b::Device::MediaType) const (this=0xd5b990, list=..., mediaType=K3b::Device::MEDIA_UNKNOWN) at /home/mark/Sources/k3b/libk3bdevice/k3bdevice.cpp:3254
#2 0x00007ffff670069e in K3b::Device::Device::determineSupportedWriteSpeeds() const (this=0xd5b990)
at /home/mark/Sources/k3b/libk3bdevice/k3bdevice.cpp:3213
#3 0x00007ffff6700317 in K3b::Device::Device::determineMaximalWriteSpeed() const (this=0xd5b990)
at /home/mark/Sources/k3b/libk3bdevice/k3bdevice.cpp:3164
#4 0x00007ffff66f2a28 in K3b::Device::Device::init(bool) (this=0xd5b990, bCheckWritingModes=true)
at /home/mark/Sources/k3b/libk3bdevice/k3bdevice.cpp:427
#5 0x00007ffff670e96d in K3b::Device::DeviceManager::addDevice(K3b::Device::Device*) (this=0xaaa500, device=0xd5b990)
at /home/mark/Sources/k3b/libk3bdevice/k3bdevicemanager.cpp:377
#6 0x00007ffff670e887 in K3b::Device::DeviceManager::addDevice(Solid::Device const&) (this=0xaaa500, solidDevice=...)
at /home/mark/Sources/k3b/libk3bdevice/k3bdevicemanager.cpp:365
#7 0x00000000004b88d3 in K3b::AppDeviceManager::addDevice(Solid::Device const&) (this=0xaaa500, solidDev=...)
at /home/mark/Sources/k3b/src/k3bappdevicemanager.cpp:119
#8 0x00007ffff670dce2 in K3b::Device::DeviceManager::checkDevice(Solid::Device const&) (this=0xaaa500, dev=...)
at /home/mark/Sources/k3b/libk3bdevice/k3bdevicemanager.cpp:251
#9 0x00007ffff670dc4b in K3b::Device::DeviceManager::scanBus() (this=0xaaa500)
at /home/mark/Sources/k3b/libk3bdevice/k3bdevicemanager.cpp:239
#10 0x00007ffff7a2990c in K3b::Core::init() (this=0x965f00) at /home/mark/Sources/k3b/libk3b/core/k3bcore.cpp:206
#11 0x000000000046dc09 in K3b::Application::Core::init() (this=0x965f00)
at /home/mark/Sources/k3b/src/k3bapplication.cpp:261
#12 0x00000000005af68a in K3b::Application::Core::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (_o=0x965f00, _c=QMetaObject::InvokeMetaMethod, _id=2, _a=0xe400d0)
at /home/mark/Sources/k3b/build/src/k3b_autogen/EWIEGA46WW/moc_k3bapplication.cpp:196
#13 0x00007fffeef5c8f1 in QObject::event(QEvent*) () at /opt/qt5/lib/libQt5Core.so.5
#14 0x00007ffff2622aec in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /opt/qt5/lib/libQt5Widgets.so.5
#15 0x00007ffff2629df1 in QApplication::notify(QObject*, QEvent*) () at /opt/qt5/lib/libQt5Widgets.so.5
#16 0x00007fffeef305e8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /opt/qt5/lib/libQt5Core.so.5
#17 0x00007fffeef3321d in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) ()
at /opt/qt5/lib/libQt5Core.so.5
#18 0x00007fffeef841b3 in () at /opt/qt5/lib/libQt5Core.so.5
#19 0x00007fffe8558fb7 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#20 0x00007fffe85591e8 in () at /usr/lib/libglib-2.0.so.0
---Type <return> to continue, or q <return> to quit---
#21 0x00007fffe855928c in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#22 0x00007fffeef8380f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
at /opt/qt5/lib/libQt5Core.so.5
#23 0x00007fffeef2ec0a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /opt/qt5/lib/libQt5Core.so.5
#24 0x00007fffeef37504 in QCoreApplication::exec() () at /opt/qt5/lib/libQt5Core.so.5
#25 0x000000000048dd6d in main(int, char**) (argc=1, argv=0x7fffffffdec8) at /home/mark/Sources/k3b/src/main.cpp:148
Mark
Git commit 7f0be6a33b8260f7789c6aeed58be8d1c844229a by Leslie Zhai. Committed on 01/08/2017 at 06:13. Pushed by lesliezhai into branch 'master'. Fix K3b::Device::from2Byte out-of-bounds issue. A great bug report by Mark! M +12 -12 libk3bdevice/k3bdeviceglobals.cpp M +3 -1 tests/k3bdeviceglobalstest.cpp https://commits.kde.org/k3b/7f0be6a33b8260f7789c6aeed58be8d1c844229a Unfortunately the fix causes a new problem. All is fine until opening the burn dialog which then says "Please insert an empty disc..." Medium Info shows no supported writing speeds.
Reverting k3bdeviceglobals.cpp 217 to:
if (d==NULL) {
plus my original hack has it working again.
I can spend some time this weekend looking at it in more detail so any clues you could give me as to what to look for would help.
(In reply to Mark from comment #4) > Unfortunately the fix causes a new problem. All is fine until opening the > burn dialog which then says "Please insert an empty disc..." Medium Info > shows no supported writing speeds. > > Reverting k3bdeviceglobals.cpp 217 to: > if (d==NULL) { > > plus my original hack has it working again. only check whether or not `d` is NULL pointer is not enough! there is out-of-bounds hidden danger when strlen(d) is 1, then d[1] will out-of-bounds. > > I can spend some time this weekend looking at it in more detail so any clues > you could give me as to what to look for would help. (In reply to Leslie Zhai from comment #5) Spent a couple hours on this but made no progress. Followed the code back to modeSense trying to see actual results from the drive but didn't help. Your fix makes perfect sense to me. With my local hack it all runs perfectly well including speed selection which I presume is from getSupportedWriteSpeedViaGP(). So I am happy to finish with this. Thanks for your help Mark Commit 7f0be6a33b8260f7789c6aeed58be8d1c844229a seems to have broken detection of the burn device / existing empty media to burn on.
I get a lot of lines printing "Invalid Byte!" the other errors:
[...]
Invalid Byte!
(K3b::Device::Device) "/dev/sr0" : Missing modepage 0x05 data.
(K3b::Device::Device) "/dev/sr0" : Cannot check write modes.
[...]
Looking at the code:
from{2,4}Byte seems to be used to read 16/32bit big endian integer values from raw data.
So calling strlen is just wrong. This leads to rejecting valid byte-streams containing zero bytes.
Second: I suggest to improve the unit test with a test parsing good 16 and 32bit values (at least one with all values != 0 and one where 0 values are part).
e.g. for from2Byte
+ unsigned const char buf0[] = { 0x00, 0x00 };
+ QCOMPARE(K3b::Device::from2Byte(buf0), (quint16)0x0000);
+ unsigned const char buf1[] = { 0x00, 0x70 };
+ QCOMPARE(K3b::Device::from2Byte(buf1), (quint16)0x0070);
+ unsigned const char buf2[] = { 0x05, 0x00 };
+ QCOMPARE(K3b::Device::from2Byte(buf2), (quint16)0x0500);
+ unsigned const char buf3[] = { 0xF0, 0x03 };
+ QCOMPARE(K3b::Device::from2Byte(buf2), (quint16)0xF003);
the same should be done for from4Byte
+ unsigned const char buf0[] = { 0x00, 0x00, 0x00, 0x00 };
+ QCOMPARE(K3b::Device::from4Byte(buf0), (quint32)0x00000000);
+ unsigned const char buf0[] = { 0x00, 0x00, 0x00, 0x01 };
+ QCOMPARE(K3b::Device::from4Byte(buf0), (quint32)0x00000001);
+ unsigned const char buf0[] = { 0x12, 0x34, 0x56, 0x78 };
+ QCOMPARE(K3b::Device::from4Byte(buf0), (quint32)0x12345678);
Right, let's put this puppy to bed once and for all ;P
Firstly, k3bdeviceglobals.cpp line 215- can revert to:
quint16 K3b::Device::from2Byte(const unsigned char* d)
{
if (d == NULL) {
qWarning() << "Invalid Byte!";
Now, the actual fix. data.size() returns (for me) 34 so (34 - 32 - 8) becomes negative which is then assigned to an unsigned int.
Here's my workaround:
k3bdevice.cpp 3241
// Ensure number of descriptors claimed actually fits in the data
// returned by the mode sense command.
if ((data.size() - 32 - 8) > 0) {
if( static_cast<int>( numDesc ) > ((data.size() - 32 - 8) / 4) )
numDesc = (data.size() - 32 - 8) / 4;
}
cd_wr_speed_performance* wr = (cd_wr_speed_performance*)mm->wr_speed_des;
Bug status does not match reality (not fixed). Not sure what status to set. Please just leave it how I set it... Git commit 16576d6fd33fd4fd81f66904d29a4478a91a561f by Leslie Zhai.
Committed on 14/08/2017 at 03:35.
Pushed by lesliezhai into branch 'master'.
Revert from{2,4}Byte.
Hi Thomas:
Please help me! I have no idea why num_wr_speed_des (numDesc) might
be wrong? and how to make it correct if bigger than (32 - 8) / 4,
and where the *magic numbers* about 32 and 8 come from.
Testcase by Matthias Schwarzott!
CCMAIL: scdbackup@gmx.net
M +11 -8 libk3bdevice/k3bdevice.cpp
M +2 -2 libk3bdevice/k3bdeviceglobals.cpp
M +19 -5 tests/k3bdeviceglobalstest.cpp
https://commits.kde.org/k3b/16576d6fd33fd4fd81f66904d29a4478a91a561f
> Firstly, k3bdeviceglobals.cpp line 215- can revert to: > > quint16 K3b::Device::from2Byte(const unsigned char* d) > { > if (d == NULL) { > qWarning() << "Invalid Byte!"; https://github.com/KDE/k3b/commit/4c8e704345a75076ef639dd748483512a7c4d52d > > Now, the actual fix. data.size() returns (for me) 34 so (34 - 32 - 8) > becomes negative which is then assigned to an unsigned int. what 32 stands for? and why minus 8? where the *magic number* comes from? > > Here's my workaround: > k3bdevice.cpp 3241 > > // Ensure number of descriptors claimed actually fits in the data > // returned by the mode sense command. > if ((data.size() - 32 - 8) > 0) { > if( static_cast<int>( numDesc ) > ((data.size() - 32 - 8) / 4) ) > numDesc = (data.size() - 32 - 8) / 4; > } > > cd_wr_speed_performance* wr = (cd_wr_speed_performance*)mm->wr_speed_des; (In reply to Leslie Zhai from comment #12) > Git commit 16576d6fd33fd4fd81f66904d29a4478a91a561f by Leslie Zhai. > Committed on 14/08/2017 at 03:35. > Pushed by lesliezhai into branch 'master'. > > Revert from{2,4}Byte. > > Hi Thomas: > Please help me! I have no idea why num_wr_speed_des (numDesc) might > be wrong? and how to make it correct if bigger than (32 - 8) / 4, > and where the *magic numbers* about 32 and 8 come from. > > Testcase by Matthias Schwarzott! > > CCMAIL: scdbackup@gmx.net > > M +11 -8 libk3bdevice/k3bdevice.cpp > M +2 -2 libk3bdevice/k3bdeviceglobals.cpp > M +19 -5 tests/k3bdeviceglobalstest.cpp > > https://commits.kde.org/k3b/16576d6fd33fd4fd81f66904d29a4478a91a561f This testcase should be deleted (for from2Byte and from4Byte): + const unsigned char buf0[] = { '\0' }; + QCOMPARE(K3b::Device::from4Byte(buf0), (quint32)0); It reads data after the array, so will observe undefined behaviour. Git commit 96540735dbcc3189812e6d46cf7d147f006e5654 by Leslie Zhai. Committed on 14/08/2017 at 07:06. Pushed by lesliezhai into branch 'master'. [tests] Remove some testcase might lead UB issue. Testcase by Matthias Schwarzott! M +14 -18 tests/k3bdeviceglobalstest.cpp https://commits.kde.org/k3b/96540735dbcc3189812e6d46cf7d147f006e5654 Hi,
Mark's workaround justifies itself by mere arithmetics. The new "if"
prevents computation from being performed if it would yield 0 or less.
The remaining question is why this computation is made and whether
it is correct.
And if it is correct, then why can it become negative at all ?
So it would be interesting to see the SCSI transaction around MODE SENSE
for page 2A from the drive which caused the segfault:
cdrskin -V dev=/dev/sr0 2>&1 >/tmp/cdrskin_scsi_log
This yields for me with a CD-RW medium in /tmp/cdrskin_scsi_log
MODE SENSE
5a 00 2a 00 00 00 00 00 4c 00
From drive: 76b
00 4a 21 00 00 00 00 00 2a 42 3f 37 f1 77 29 23 1b 90 01 00
0f e0 10 8a 00 10 06 e4 06 e4 00 01 00 00 00 00 06 e4 00 01
00 00 06 e4 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Please post the whole file. There will be more than one MODE SENSE in it.
------------------------------------------------------------------------
Now let's see what K3B is doing this time what it should leave to burn
programs. :))
getSupportedWriteSpeedsVia2A() requests from the drive Mode Page 2A
"MM Capabilities and Mechanical Status Page". It is obsoleted since MMC-4,
but afaik still supported by all drives. (More modern is command
ACh GET PERFORMANCE in K3b::Device::Device::getPerformance().)
If a Mode Page is fetched by command 5Ah MODE SENSE(10), then it is preceeded
by a header struct of 8 bytes (SPC-3, table 240). This header tells in its
bytes 0 to 2 the number of bytes minus 2 which can at most be replied for
the given mode page.
From
mm_cap_page_2A* mm = (mm_cap_page_2A*)&data[8];
i conclude that "data" is the raw reply from the drive which now gets
overlaid by a C++ struct. (So i can only guess the mapping of specs to code,
from here on.)
Mode page 2A is described in MMC-3, 6.3.11.
In page 2A the number of speed descriptors is recorded in bytes 30 an 31
of the replied page. Their size is four bytes each. So in above example
from cdrskin there are
0x4a + 2 - 8 - 32 = 36
descriptor bytes.
I never encountered drives which tell descriptor bytes rather than
descriptor count. Can this be a rumor caused by other problems ?
There is some contradiction in
if( data.size() > 32 ) {
// we have descriptors
versus
numDesc = (data.size() - 32 - 8) / 4;
The "if" assumes that data.size() is the net mode page size.
The "numDesc" computation assumes that it is the combined size of header
and page.
Looking at k3bdevice_mmc.cpp line 676
bool K3b::Device::Device::modeSense( UByteArray& pageData, int page ) const
i get the impression that it is a generic function of the array and
reflects its size as recorded by
pageLen = 8;
if( cmd.transport( TR_DIR_READ, header, 8 ) == 0 )
pageLen = from2Byte( header ) + 2;
...
pageData.resize( pageLen );
So this is wrong in getSupportedWriteSpeedsVia2A() :
if( data.size() > 32 ) {
// we have descriptors
unsigned int numDesc = from2Byte( mm->num_wr_speed_des );
It should be
if( data.size() > 8 + 32 ) {
// we have descriptors
unsigned int numDesc = from2Byte( mm->num_wr_speed_des );
With the wrong comparison and if there are no descriptors, the value numDesc
might be taken from bytes which were not allocated by modeSense().
(Doesn't the fancy UByteArray have an overflow protection ?)
I now ponder whether this might be the reason for the rumor:
// Some CDs writer returns the number of bytes that contain
// the descriptors rather than the number of descriptors
Have a nice day :)
Thomas
Hi, the proposed cdrskin run has a wrong redirection sequence. It must be cdrskin -V dev=/dev/sr0 >/tmp/cdrskin_scsi_log 2>&1 (I tested mine with "| less". When changing this to "> /tmp/..." i stepped into the well known shell redirection puddle. Duh.) Have a nice day :) Thomas Git commit d50a0c30b582aa2fc5cfccea4b87a70adc2d5d81 by Leslie Zhai. Committed on 14/08/2017 at 08:21. Pushed by lesliezhai into branch 'master'. Fix wrong comparison if there are no descriptors. A patch by Thomas Schmitt! Testplan by Mark: cdrskin -V dev=/dev/sr0 2>&1 >/tmp/cdrskin_scsi_log M +2 -6 libk3bdevice/k3bdevice.cpp https://commits.kde.org/k3b/d50a0c30b582aa2fc5cfccea4b87a70adc2d5d81 Hi, i was not really in patch mood yet, but still in the stage of making up theories. Nevertheless, the new commit should really fix the problem. Mark reports in https://bugs.kde.org/show_bug.cgi?id=382941#c8 that he gets data.size() == 34. Minus 8 header bytes this is mode page length 26. This means 6 mandatory bytes from MMC-3 table 361 are missing. In MMC-1 table 103 the mode page 2Ah is only 22 bytes long. MMC-2 table 137 has indeed 26 bytes. It obsoletes half of the MMC-1 definition. E.g. "Maximum Write Speed Supported" and "Current Write Speed Selected" at bytes 18 to 21. MMC-3 then adds again a field "Current Write Speed Selected" at byte offset 28 and the list of speed descriptors. So yes, with page length 26 and the wrong "if" condition, K3B picked two bytes as numDesc by unsigned int numDesc = from2Byte( mm->num_wr_speed_des ); which were not read from the reply of command MODE SENSE. It did not throw any error towards Mark when it was doing this. The error happened later when numDesc was the rolled-over unsigned representation of the negative number and caused a long and rampant loop until segfault by a large index "i". --------------------------------------------------------------------- But i still riddle about the original cause to make the problematic computation. It is not easy to imagine how a K3B developer could have mistaken the random number for a byte count and compute the descriptor count from the reply size without noticing that it becomes negative and then a very large unsigned int. So maybe this gesture is not a reaction to MMC-1 or MMC-2 compliant replies but rather a reaction to some other unexpected situation. I still doubt that any drive would really announce a byte count of the descriptor list rather than the item count of that list. But given my current assessment, i cannot recommed to remove this fallback computation. (I would like to. Really.) Have a nice day :) Thomas Hi,
some final remarks about the comments:
-----------------------------------------------------------------------
This rumor can be deleted. It is not relevant in any way:
// Some CDs writer returns the number of bytes that contain
// the descriptors rather than the number of descriptors
Whereas this part is a wise precaution against mad drive replies of any
kind:
// Ensure number of descriptors claimed actually fits in the data
// returned by the mode sense command.
if (static_cast<int>(numDesc) > ((data.size() - 32 - 8) / 4))
numDesc = (data.size() - 32 - 8) / 4;
-----------------------------------------------------------------------
The new comment in
if (data.size() > 32 + 8/* pageLen? */) {
does not explain the numbers 8 and 32. pageLen is a misnomer for data.size().
A better name in modeSense() would be "replyLen".
How about being more verbous:
// 8 bytes of MODE SENSE(10) header
// 32 bytes offset of speed descriptor list in Mode page 2Ah
// (MMC-3, table 361. Pages of MMC-1 and MMC-2 are smaller.)
if (data.size() > 8 + 32) {
// we have descriptors
It is necessary to mention MODE SENSE(10), because MODE SENSE(6) yields a
header length of 4.
-----------------------------------------------------------------------
Have a nice day :)
Thomas
Git commit 8cf67cf6ec6312fb82cdca729fc03e2acea22989 by Leslie Zhai. Committed on 15/08/2017 at 01:41. Pushed by lesliezhai into branch 'master'. [libk3bdevice] Update comment for modeSense M +8 -5 libk3bdevice/k3bdevice.cpp M +25 -25 libk3bdevice/k3bdevice_mmc.cpp https://commits.kde.org/k3b/8cf67cf6ec6312fb82cdca729fc03e2acea22989 Thank you for the crash report. As it has been a while since this was reported, can you please test and confirm if this issue is still occurring or if this bug report can be marked as resolved. I have set the bug status to "needsinfo" pending your response, please change back to "reported" or "resolved/worksforme" when you respond, thank you. Dear Bug Submitter, This bug has been in NEEDSINFO status with no change for at least 15 days. Please provide the requested information as soon as possible and set the bug status as REPORTED. Due to regular bug tracker maintenance, if the bug is still in NEEDSINFO status with no change in 30 days the bug will be closed as RESOLVED > WORKSFORME due to lack of needed information. For more information about our bug triaging procedures please read the wiki located here: https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging If you have already provided the requested information, please mark the bug as REPORTED so that the KDE team knows that the bug is ready to be confirmed. Thank you for helping us make KDE software even better for everyone! This bug has been in NEEDSINFO status with no change for at least 30 days. The bug is now closed as RESOLVED > WORKSFORME due to lack of needed information. For more information about our bug triaging procedures please read the wiki located here: https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging Thank you for helping us make KDE software even better for everyone! |
Created attachment 106978 [details] Out from k3b and cdrecord -prcap Number of supported write speeds via 2A: 4294967295. Attached is console out from k3b. Hack to get it running: k3bdevice.cpp ln 3235: if( !data.size() > 32) { Drive Model: GCE-8525B