Re: [Libcdio-devel] W32 problems

[Rocky Bernstein]

On Sun, Jan 20, 2013 at 11:52 AM, LRN <address@hidden> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
>
> Tried 0.90 release.
>
Compiles out of the box ok.
> Testuite runs OK OOTSD, all tests pass, except for testisocd.
> 3 tests were skipped:
>
> testgetdevices test skipped until drive recording testing issues resolved
> SKIP: testgetdevices.exe
>
> - -- Unable find or access a CD-ROM drive with an ISO-9660 filesystem.
> SKIP: testisocd.exe
>
> SKIP: check_cue.sh
>
>
> mmc3 passed, but printed some warnings.
>
>
> The crash is, i think, due to run_mmc_cmd_win32ioctl(). Again.
> See, it has this stack-allocated
>   SCSI_PASS_THROUGH_WITH_BUFFERS sptwb;
> structure, its size is 592 (it has a 512-bytes buffer in it).
>
> Then you do:
> if (SCSI_MMC_DATA_WRITE == e_direction) memcpy(&sptwb.DataBuf, p_buf,
> i_buf);
> Which is alarming, since there is no guarantee (AFAICS) that i_buf
> does not exceed 512.
>
> Then you do:
>   sptwb.Spt.DataTransferLength= i_buf;
> even though sptwb.DataBuf is only 512 bytes, while i_buf can be longer.
>
> Then you calculate length as:
>   length = offsetof(SCSI_PASS_THROUGH_WITH_BUFFERS, DataBuf) +
>     sptwb.Spt.DataTransferLength;
>
> which assumes that sptwb is followed by a
> DataTransferLength-bytes-long buffer, even though it's only 512 bytes
> long.
>
> And finally:
>
>   b_success = DeviceIoControl(p_env->h_device_handle,
>                               IOCTL_SCSI_PASS_THROUGH,
>                               (void *)&sptwb,
>                               sizeof(SCSI_PASS_THROUGH),
>                               &sptwb,
>                               length,
>                               &dw_bytes_returned,
>                               NULL);
>
> which writes back "length" bytes into &sptwb, corrupting the stack.
>

Ok. Thanks for the explanation. I think I have have seen this kind of crash
on Windows.

I'll try address this when I get a chance, probably by allocating sptwb
based on the size passed in, i_buf. For various specific MMC commands there
are limits on data size.  And what I've noticed is on some situations is
that MS Windows doesn't exceeds these.

But either we should check against that limit which is not done here, or
allow for no limit by allocating accordingly

Right now though I have a number of other things to focus on so this is
pretty low priority. So if you want to suggest a patch, this might speed
things along.


>
> Test log (without USE_PASSTHROUGH_DIRECT) is attached.
>
>
> Tried rebuilding with CPPFLAGS="-DUSE_PASSTHROUGH_DIRECT=1".
> testisocd did not crash, and passed, but instead mmc_read failed:
> Error: Sense key should be 5, got 0
> FAIL: mmc_read.exe
>
>
>
> Also, suggestion:
> in default_cdio_log_handler() use this code:
>
> #include <windows.h>
> DebugBreak ();
> ExitProcess (3);
>
> instead of
> abort ();
>
> The reason for this is that abort() in MS C Runtime library simply
> prints this:
>
> This application has requested the Runtime to terminate it in an
> unusual way.
> Please contact the application's support team for more information.
>
> to stderr, then calls raise(SIGABRT), which causes the process to
> terminate abnormally (exit code 3), without flushing, closing, or
> doing any cleanups.
>
> The problem in all that is that gdb is not able to catch SIGABRT on
> W32. Thus if you are debugging a process, and the process calls
> abort(), it will die right under you, never giving you a chance to see
> or debug the place where problem happened.
>
> Thus i prefer to call DebugBreak(), which throws and exception that
> gdb (or any debugger) can catch. If there's no debugger attached,
> exception will go unhandled, crashing the process normally.
>
> ExitProcess() is there to ensure that the process won't continue after
> DebugBreak () if debugger is present (or exception was handled somehow).
>

Seems reasonable. Work up a patch for this.


>
> If you don't like the idea of always using DebugBreak() (don't know
> why anyone wouldn't like it, probably a religious thing...),


If someone objects, then they'll probably let us know and give a reason why.
Until then, let's make this more amenable for folks like you who use this
and are willing to dig into it when there is a problem.


> then call
> IsDebuggerPresent() first, to determine whether the process is being
> debugger, and only call DebugBreak() if it is. Otherwise call abort().
>



> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (MingW32)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQEcBAEBAgAGBQJQ/CCvAAoJEOs4Jb6SI2Cw2JsH/R9Zby4zR0iUA1vxS4krpl0I
> 2n/QwaBNBhj6n0pUkwcxOUqToV/RvyE3WWwyUh+DPRCF/h7HfL8uaBbwJFJ97W42
> Dsu0gIWIbfz3IZPc//IG3QCnwJY8E9w6MgM2sXynVjaFKTebZbesB+gaEKGj8XC6
> +PQ01qfBBZ2Yd1XGl1HM3dqcdfZIfpD5cFgXVXwbwpVb8XQAuP5ONpZybcJC8560
> /emLo1XhrPsRLlwkMjqg7+rzWJCQFOjhXNgkH+dvgXHul3it26jndgMqM0BncieI
> DxRF1RN/zZ/jBpg8CjGZVCinHeWX86dRJHg43R2RrFGJ7SZFDwwL+eCrNN2U0WE=
> =gfY8
> -----END PGP SIGNATURE-----
>