"
bool ReadMBRUsingScsiPassThruDirect10withSense(int i, std::vector&
vIoctlSector)
> {
> BOOL status = 0;
NO! That would be
BOOL status = FALSE;
> DWORD accessMode = 0, shareMode = 0;
Use of commas in declarations makes for unreadable code. One variable,
one line.
Why do you need these as variables anyway?
Furthermore, you don’t actually use them anywhere I can see.
> ULONG alignmentMask = 0; // default == no alignment requirement
PUCHAR dataBuffer = NULL;
> PUCHAR pUnAlignedBuffer = NULL;
> SCSI_PASS_THROUGH_DIRECT_WITH_BUFFER sptwd;
> int sectorSize = 512;
>
This may not work for all drives, because sector sizes of 4096 can exist.
I would think the best strategy is to query the drive for its sector size.
I see later you query the disk geometry; why aren’t you passing the sector
size value in?
> HANDLE phDisk = NULL;
> phDisk=
> CreateFile(L"\\.\physicaldrive0",GENERIC_READ|GENERIC_WRITE,FILE_SHARE_READ
| FILE_SHARE_WRITE,NULL,OPEN_EXISTING,0,NULL);
> if(phDisk == INVALID_HANDLE_VALUE)
> return false;
I note that here you check the failure mode correctly, but elsewhere you
seem to assume that if CreateFile fails it will return a NULL handle,
which is incorect, and the code is therefore totaly nonsense.
And why did you call the variable a “pointer to handle” (ph) when it is
just a handle (h)? If you can’t use Hungarian Notation correctly, don’t
use it at all, because incorrect usage makes the program hard to read.
>
> GetAlignmentMaskForDevice(phDisk,&alignmentMask);
>
> ZeroMemory(&sptwd, sizeof(SCSI_PASS_THROUGH_DIRECT_WITH_BUFFER));
> dataBuffer = AllocateAlignedBuffer(sectorSize,alignmentMask,
> &pUnAlignedBuffer);
It may have been an artifact of all the comments I’ve been adding, or the
email system, but the databuffer= assignment appeared on the same line as
the ::ZeroMemory. This is poor style if that is how it was orginallly
written.
Why do this if the alignment mask plus the buffer address show that the
data is properly aligned?
>
> ZeroMemory(dataBuffer,sectorSize);
Since you know about ::ZeroMemory, why do you do a lot of memsets
elsewhere? For that matter, why are you zeroing a buffer that is going to
be completely overwritten?
>
> sptwd.sptd.Length = sizeof(SCSI_PASS_THROUGH_DIRECT);
> sptwd.sptd.CdbLength = CDB10GENERIC_LENGTH;
> sptwd.sptd.DataIn = SCSI_IOCTL_DATA_IN;
> sptwd.sptd.SenseInfoLength = SPT_SENSE_LENGTH;
> sptwd.sptd.DataTransferLength = sectorSize;
> sptwd.sptd.TimeOutValue = 50;
> sptwd.sptd.DataBuffer = dataBuffer;
> sptwd.sptd.SenseInfoOffset =
> offsetof(SCSI_PASS_THROUGH_DIRECT_WITH_BUFFER,ucSenseBuf);;
>
> sptwd.sptd.Cdb[0] = 0x28;
> sptwd.sptd.Cdb[1] = 0x04; //Set to force unit access so that
cached
> data is not used.
And this flag does not have a symbolic name? Seriously, now! And what is
the meaning of the magical constant 28? Note that you could have done
something like
#define MyFlags (something | other | whatever)
and written:
sptdw.sptd.Cdb[0] = HIBYTE(MyFlags);
sptdw.sptd.Cdb[1] = LOBYTE(MyFlags);
which is converting from little-endian notation to big-endian notation, or
swap LOBYTE and HIBYTE in the above to store the 16-bit flag value in
little-endian notation. In either case, the meaning is clear. The
meaning of 0x28, 0x4 is NOT clear, and I have no idea what is going on or
is intended.
> sptwd.sptd.Cdb[2] = (i >> 24 ) & 0xFF;
> sptwd.sptd.Cdb[3] = (i >> 16 ) & 0xFF;
> sptwd.sptd.Cdb[4] = (i >> 8 ) & 0xFF;
> sptwd.sptd.Cdb[5] = (i>> 0 ) & 0xFF;
What happens if i is a negative value? Note that you should not use “int”
if you expect an unsigned value. Also, it looks like you are converting
the value from little-endian to big-endian; since I am not an expert at
any of these structures, I can’t say for sure if this is correct or not,
it just jumped out at me as a possible question.
> sptwd.sptd.Cdb[6] = 0;
> sptwd.sptd.Cdb[7] = (UCHAR)(1 >> 8);
1 >> 8 is always going to be zero. Always. And if you meant <<, then 1
<< 8 cast to a UCHAR will be zero. Always.
> sptwd.sptd.Cdb[8] = 1;
> sptwd.sptd.Cdb[9] = 0;
> ULONG length = sizeof(SCSI_PASS_THROUGH_DIRECT_WITH_BUFFER); ULONG
returned = 0;
Why are there two declarations on the same line?
Note that the length is supposed to be a DWORD, and you are presuming that
the implementation of ULONG and DWORD are the same. Why are you using
different types than the documentation suggests, based on some bizarre
assumption that you know the underlying base types?
For that matter, why do you need to declare a variable for ‘length’ when
you could have simply used the sizeof() in the call below? And, if you
want to declare the variable for convenience, why did you not declare it
as const?
> status = DeviceIoControl(phDisk,
> IOCTL_SCSI_PASS_THROUGH_DIRECT,
> &sptwd,
> length,
> &sptwd,
> length,
> &returned,
> FALSE);
>
>
> if ((sptwd.sptd.ScsiStatus == 0) && (status != 0)) {
Why are you comparing a BOOL with an integer? Why do you need to compare
a BOOL to anything, anyway? It already stands for true or false. Note
also that because BOOL is a weird type introduced long before bool was in
the language, it sometimes takes on values OTHER than 0 or 1, so writing
if(somebool == TRUE)
is often erroneous. Since somebool is already TRUE or FALSE, comparing it
to TRUE or FALSE is just completely silly.
> printf(“Using IOCTL SCSI 10\n”);
> PrintDataBuffer(dataBuffer,sptwd.sptd.DataTransferLength);
vIoctlSector.resize(sptwd.sptd.DataTransferLength);
> memcpy(vIoctlSector.data(), dataBuffer,
> sptwd.sptd.DataTransferLength);
> }
> // free(pUnAlignedBuffer);
So, having allocated the buffer, why do you not free it? You have a
storage leak.
> DWORD error = GetLastError();
Since this appears to be user-level code, you can use the display of
ERR,hr in your watch-window to look at the GetLastError code for whatever
thread broke into the debugger, and this also gives you the text of the
error message. But since you have no idea whether or not the API
succeeded, and printf or PrintDataBuffer might have changed the value,
reading it here is completely nonsensical. If an API fails, the FIRST
think you do is read the error code; after that point, you cannot assume
its integrity. If the API succeeded, with a very few rare exceptions, the
value of GetLastError() is undefined.
> CloseHandle(phDisk);
> return true;
> }
What problem are you trying to solve? Note that computing the “offset” by
simple sector arithmetic does presume that the “file” is not fragmented.
If there is fragmentation, or the sector size is not 512, then I’m not
sure what you are going to get, but I suspect it is not what you are
asking for.
Code like this, that makes presumptions about what is found on the hard
drive and where it is found, is potentially very fragile. It helps a bit
to have some idea what the goal of this code is, so that some hard drive
expert can say, “Oh, that’s not quite the way to do it” or “Sure, that’s
correct, if the second size really is 512” or some other suitable
response.
joe
>
>
> ATA:
>
> bool ReadMBRUsingATAPassthru(int i, std::vector& vATA)
> {
> HANDLE handle;
>
> UCHAR buffer[512] = {0};
Why do you assume the second size is 512? You have no reason to make this
assumption, and you actually query the disk geometry to obtain the value,
which you are ignoring here. Note that you cannot declare a local array
with a dynamic size, but why do you need the local array? Why do you
declare the std::vector to be a vector of BYTEs but declare buffer as a
UCHAR (sure, they’re the same, but really? Two separate names?)
I would have done
vATA.resize(sectorsize); // which must be passed in as a parameter
LPBYTE buffer = &vATA[0];
This also eliminates the need to do the completelly unnecessary memcpy
operation.
>
> DWORD bytesReturned = 0;
>
> DWORD outBufferSize = sizeof(ATA_PASS_THROUGH_DIRECT);
And you needed to declare a gratuitous variable for what purpose?
>
> int bytescopied = 0;
This does not appear to be used
>
>
> WCHAR fileName = (WCHAR * ) “\\.\PHYSICALDRIVE0”;
This is just plain WRONG! The correct statement is
LPWStr filename = L"\\.\PHYSICALDRIVE0";
You have taken an 8-bit character string and told the program to interpret
it as a set of pairs of bytes defining aa Unicode character, and the
result will be total nonsense.
>
>
>
> int errorCode = 0;
>
> int bResult = 0;
WTF? a “bResult” cannot possibly be an int. Even the Hungarian Notation
suggests it is a BOOL, or possibly a bool, so it can only get TRUE/FALSE
or true/false. Your function returns a bool, not an int, so why are you
setting this int to a numeric value? Why do you have this horrid
fascination of representing everything as signed values when I can’t find
a single place where these values are used as signed values.
>
>
> int ioControlCode = IOCTL_ATA_PASS_THROUGH_DIRECT;
>
Why do you assign a constant to ioControlCode? Why, for goodness sake, do
you make it an “int”? RTFM. The DeviceIoControl API takes a DWORD for
this parameter! But since you use this in only one place, its existence
as a separate variable is nonsense. Just use the constant IOCTAL_ATA_…T
in the call. Or are you one of the people who believe that because the
API definition says DWORD ioctlCode that you MUST have a DWORD variable
declared to represent it (that’s not what the API definition says; it says
that the parameter must evaluate to a DWORD, which means using the
constant in the DeviceIoControl call is perfectly valid)
‘int’ is rarely your friend when you are programming at this low level.
‘int’ is not always your friend in ordinary application code. Note that
‘int’ is ALWAYS 32 bits, even on 64-bit platforms.
> unsigned int inputBufferSize = sizeof(ATA_PASS_THROUGH_DIRECT);
Why ‘unsigned int’ instead of ‘DWORD’? Those types exist for a reason.
>
> ATA_PASS_THROUGH_DIRECT inputBuffer = {0};
> memset(&inputBuffer,0,40);
Why 40? Why not sizeof() ATA_PASS_…T or InputBuffer? And why are you
zeroing out a buffer you have declared and initialized to be full of
zeroes? And why are you using the rather primitive memset when there is a
::ZeroMemory API? (sure, it’s just a macro that translates to memset, but
using memset is almost-but-not-quite-as-bad as doing an assembly-code
insertion, given that ::ZeroMemory already exists)
And why do you think 40 == sizeof(ATA_PASS_THROUGH_DIRECT) anyway?
> memset(buffer, 0, 512);
Why 512? Ditto. And why are you zeroing it out?
>
> inputBuffer.AtaFlags = 03 | ATA_FLAGS_48BIT_COMMAND ;
And why are you writing an octal 3, when we know it is the same as a hex 3
or a decimal 3? And what does “3” mean, anyway? Isn’t there an ATA_FLAGS
constant or pair of constants that create this value?
>
> inputBuffer.Length = 40;
Didn’t you mean sizeof(ATA_PASS_THROUGH_DIRECT)?
>
> inputBuffer.DataTransferLength = 512;
What is totally bizarre is that you have gone through some effort to
detect the sector size, which you promptly proceed to ignore totally, and
assume it is 512. Why?
>
> inputBuffer.TimeOutValue = 500;
> inputBuffer.DataBuffer = buffer;
> inputBuffer.Lun = 0;
> inputBuffer.PathId = 0;
> inputBuffer.TargetId = 0;
>
>
> inputBuffer.PreviousTaskFile[0] = 0;
> inputBuffer.PreviousTaskFile[1] = 0;
> inputBuffer.PreviousTaskFile[2] = (i >> 24) & 0xFF;;
> inputBuffer.PreviousTaskFile[3] = (i >> 32 ) & 0xFF;
> inputBuffer.PreviousTaskFile[4] = (i >> 40 ) & 0xFF;;
Note that since i is a 32-bit signed integer, >> 32 and >> 40 will be zero
unless the value is negative, in which case the result will end up as
being 0xFF after the propagated size bits are stripped off by the & 0xFF.
You should not be using a signed value here for a length, and if you
expect >> to work for values > 32, you should be passing in a ULONGLONG
(unsigned __int64 on most x86 platforms).
Realistically, you will never have a negative number because that can only
arise if you have a buffer > 2GB to write, and your chances of getting
this on a Win32 platform are vanishingly small. But if i is a count of
bytes, why is it not a DWORD? See previous comments on the overuse of
‘int’. This reads like 1975 PDP-11 code.
> inputBuffer.PreviousTaskFile[5] = 0xE0;
And E0 means what?
> inputBuffer.PreviousTaskFile[6] = 0;
> inputBuffer.PreviousTaskFile[7] = 0;
>
> inputBuffer.CurrentTaskFile[0] = 0;
> inputBuffer.CurrentTaskFile[1] = 1;
> inputBuffer.CurrentTaskFile[2] = (i >> 0 ) & 0xFF;;
> inputBuffer.CurrentTaskFile[3] = (i >> 8 ) & 0xFF;;
> inputBuffer.CurrentTaskFile[4] = (i >> 16 ) & 0xFF;;
> inputBuffer.CurrentTaskFile[5] = 0xE0;
> inputBuffer.CurrentTaskFile[6] = 0x20;
> inputBuffer.CurrentTaskFile[7] = 0;
I’m curious why you have represented these as bytes, when it seems some
much richer structure is being manipulated. I might have written a struct
with fields like
DWORD / BIG-ENDIAN / whatever;
which has the additional comment that the value is to be represented as a
32-bit big-endian value; I would probably include the network header file
and use htonl to swap the bytes, e.g.,
somestructure.whatever = htonl(i);
which seems a lot cleaner. The magic constants such as 0xE0 an 0x20 also
make me nervous; I’d rather have symbolic names like
inputBuffer.Flags = PAINT_SURFACE_PURPLE | EXCLUDE_GREEN_BITS_ON_LAN
| FRAMMIS_THE_REFLECTED_VALUE | DONT_COMPLAIN_IF_IMPOSSIBLE;
instead of splitting what might be a 16-bit flags value into two
apparently-random constants. Or not. Hard to guess when you are
converting everything to a byte array.
f
Note also that I put spaces around the | so that human beings, not just
the C/C++ compiler, can read this, too.
>
> handle=
> CreateFile(L"\\.\PhysicalDrive0",GENERIC_READ|GENERIC_WRITE,FILE_SHARE_READ
| FILE_SHARE_WRITE,NULL,OPEN_EXISTING,0,NULL);
>
And what became of that LPWSTR (or WCHAR ) to which you assigned an 8-bit
string?
Put spaces around the | operator. Humans have to read this as well.
>
>
> printf(“\n Error = %d”, GetLastError());
>
Note that this value is meaningless if the CreateFile succeeded, so why
are you printing it out here?
>
>
> if(handle)
So you plan to execute this if CreateFile failed? RTFM. If CreateFile
fails, it returns INVALID_HANDLE_VALUE, not NULL. INVALID_HANDLE_VALUE,
just to know the implementation detail, is
(HANDLE)(UINT_PTR)(INT_PTR)(-1).
>
> {
>
> DeviceIoControl(
>
> handle,
>
> ioControlCode,
>
> &inputBuffer,
>
> outBufferSize,
Why use outBufferSize as the size of the input? For that matter, why did
you create a totally unnecessary variable to hold the value; and if you
feel compelled to use the variable, why was it not declared const?
>
> &inputBuffer,
>
> outBufferSize,
>
> &bytesReturned,
>
> NULL //not an overlapped operation
>
> );
>
>
>
> printf(“IOCTL ATA\n”);
> printf(“\n status is %d”,inputBuffer.CurrentTaskFile[6]);
You are printing this out even though the operation may have failed,
meaning that the value in the status is not defined. You should only
print this out if it succeeded.
>
> printf(“\n Error = %d”, GetLastError());
Again, this value is totally meaningless if the DeviceIoControl succeeded.
You should only print this out if it failed.
> PrintDataBuffer(buffer, 512);
If it failed, the data in buffer is probably meaningless, so why are you
bothering to print it out? This should only be printed out if the
operation succeeded.
> memcpy(&vATA[0],buffer, 512);
And you know that vATA points to a buffer of at least 512 bytes exactly how?
the correct code would be
memcpy(&vATA[0], buffer, min(sizeof(buffer), vATA.size()) );
and if you want to believe the buffer has 512 bytes in it, that is
expecting a lot. The correct code could also be
vATA.resize(sizeof(buffer));
memcpy(&vATA[0], vATA.size());
it is NEVER safe to assume that any buffer pointer which is passed in
represents a buffer of a valid size; if you were not using std::vector,
you would be obliged to pass in the explicit length. In some companies,
failing to check buffer boundaries is a firable offense (usually the first
time is just a stern warning). Since vATA is a std::vector, you CANNOT
assume that it has been properly sized; you must size it yourself before
copying into it, or accept the size it has. Due to errors in the calling
code, it is entirely possible the size of the vector could be zero, in
which case the memcpy is erroneous because it does not know if the target
pointer is even a valid pointer. Defer the resize() to this routine.
>
>
>
> }
> CloseHandle(handle);
> return true;
What happened to that incorrectly-declared and improperly-initialized
bResult? You are returning ‘true’ even if it failed? And you are calling
CloseHandle on a handle whose value is not known, and could be
INVALID_HANDLE_VALUE?
>
>
> }
>
> For ATA the inputBuffer.CurrentTaskFile[6] = 0x20;
> for 48 bit is 0x24 (or read Ext - but that works only till LBA 2 and 3
returns 0’s with error 81 or invalid arguments).
If error code 81 is returned, you have no idea what LBA 2 or 3 returned,
because the value is almost certainly undefined.
And what are the meanings of 0x20 or 0x24? Either they are bit fields, in
which case you should be using symbolic names, or they are truly the
numeric constants 32 and 36. It is hard to guess.
>
> Via the API:
>
> bool ReadMBRSystemDrive(int i, std::vector& vBytes, DWORD& size) {
Note that ‘size’ is unneeded. You can use vBytes.size() to carry this
information back
But you still insist on passing a length in as a signed value!
> // Open the physical hard drive
> HANDLE phDisk = NULL;
> phDisk=
> CreateFile(L"\\.\physicaldrive0",GENERIC_READ,FILE_SHARE_READ |
FILE_SHARE_WRITE,NULL,OPEN_EXISTING,FILE_ATTRIBUTE_NORMAL,NULL);
> DWORD ret = GetLastError();
> if(!(phDisk != NULL && GetLastError() ==0) )
> return false;
This code is complete and utter nonsense. The value of GetLastError() is
undefined if CreateFile succeeds. Note also that if it succeeds, and
phDisk (which is not a pointer to a handle as the name suggests; if you
can’t use HN correctly, don’t use it at all, since it only confuses the
reader) is not NULL, and some random API in history (or the occasional
library call) happened to leave the GetLastError as 0, you will return
false, which says to declare failure if it succeeds, and, by the way, if
it succeeded, you have a handle leak.
The correct code would be
HANDLE hDisk;
hDisk = CreateFile(…as above…);
if(hDisk == INVALID_HANDLE_VALUE)
{ / failed /
// …maybe some nice printout here such as the reason for the
failure…
return false;
} / failed /
> // Retrieve the sector size
> DWORD dwReturnLength = 0;
> DISK_GEOMETRY infoDiskGeometry;
> if( !::DeviceIoControl(phDisk, IOCTL_DISK_GET_DRIVE_GEOMETRY,
> NULL, 0,
> &infoDiskGeometry, sizeof(infoDiskGeometry),
> &dwReturnLength, NULL) )
> {
> DWORD dwErr = GetLastError();
> return false;
> }
See, you DO know how to write the correct code! You did it here!
> size = infoDiskGeometry.BytesPerSector;
> // A vector large enough to hold one sector
> vBytes.resize(infoDiskGeometry.BytesPerSector);
> // Read first sector from the disk
> if( SetFilePointer(phDisk, (iinfoDiskGeometry.BytesPerSector),
> NULL,FILE_BEGIN) == INVALID_SET_FILE_POINTER )
> return false;
> DWORD numberRead = 0;
> if(! ReadFile(phDisk,
> reinterpret_cast(&vBytes[0]),infoDiskGeometry.BytesPerSector,&numberRead,NULL)
)
The reinterpret_case is unnecessary since any pointer can be recast to
void (or LPVOID or PVOID, which would be the more natural expressions of
this in an app)
> {
It might be nice to do some printout of the error reason here. Also,
since you clearly have a valid open handle at this point, you should do a
::CloseHandle on it, or you have a really nice handle leak.
> return false;
> }
At this point, you know how many bytes were read, so why aren’t you
assigning it to ‘size’, the DWORD& whose apparent purpose is to return
this? Actually, I would do a resize() of the vector to be this value, so
the additional reference parameter would be unnecessary. Also, it is
worth pointing out that if the value is OTHER than the size of a sector,
there is something deeply wrong.
> CloseHandle(phDisk);
> return true;
> }
>
> And I am looping
>
> while ( true )
> {
> DWORD size = 0;
> std::vector sysDriveApi;
> ReadMBRSystemDrive( i, sysDriveApi, size);
It won’t, because readMBRSystemDrive does not set size. Also, we have no
idea what i is, since it is not declared or set anywhere. Note also that
if ReadMBRSystemDrive fails, you keep going ahead and running the program,
instead of printing an error message and exiting the loop. Why?
> std::vector sysDriveScsi (size);
Since size is not set, this will create a buffer of size 0, which you
will then pass in and blindly use, and since the meaning of
&sysDriveScsi[0] is meaningless if sysDriveScsi.size() == 0, nothing but
total disaster can follow.
> ReadMBRUsingScsiPassThruDirect10withSense(i, sysDriveScsi);
std::vector sysDriveAta (size );
Why does readMBRUsing…Sense have any right to presume that sysDriveScsi
has a buffer of valid length? See previous comment. You cannot trust the
value to be meaningful in the routine, and it is poor programming practice
to make the assumption that it is correct.
> ReadMBRUsingATAPassthru( i, sysDriveAta);
> if( memcmp(&sysDriveApi[0],&sysDriveScsi[0], size) != 0 ||
> memcmp(&sysDriveApi[0],&sysDriveAta[0], size) != 0 )
Both terms of the above disjunction are identical!!!
> {
> printf(" LBA %d does not match \n", i);
> PrintDataBuffer(reinterpret_cast(&sysDriveApi[0]),
> size);
> PrintDataBuffer(reinterpret_cast(&sysDriveScsi[0]),
> size);
> PrintDataBuffer(reinterpret_cast(&sysDriveAta[0]),
> size);
Why doesn’t PrintDataBuffer take a std::vector & argument? Why all
this clumsiness at the call site?
> if( memcmp( &sysDriveScsi[0], &sysDriveAta[0], size) != 0) {
> printf(" LBA SCSI/ATA %d does not match \n", i);
> }
> //break;
> }
> i++;
I find the i++ to be bizarre. If this is the offset, incrementing it by 1
is weird. If, on the other hand, it is the LBA, then it would not be a
signed int, but it would have a more meaningful name than “i”, e.g.
DWORD LBAnumber = 0;
and
LBAnumber++;
would make it clear what is going on.
>
> }
>
> With 0x24 in the ATA command it breaks at LBA 3 with ATA returning 0’s
and
> error 81.
> With 0x20, it fails on LBA 3f where SCSI and ATA match but are both
incorrect.
>
> Please note this is just working code (it has not been cleaned up and
might have some minor inconsistencies).
>
> I would appreciate any guidance/ test code that might be pointed to.
>
> Thanks,
> Sonia.
>
>
>
> —
> NTDEV is sponsored by OSR
>
> Visit the list at: http://www.osronline.com/showlists.cfm?list=ntdev
>
> OSR is HIRING!! See http://www.osr.com/careers
>
> For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars
>
> To unsubscribe, visit the List Server section of OSR Online at
> http://www.osronline.com/page.cfm?name=ListServer
>