Fix for 2012 bug! Original code zeroed al and copied the class

[the-grue]

into this register then did comparisons against ah instead of al. Prior to this instruction, ax had the date in it in directory entry format. With the shift in ah for the year going to the 32 as well as the month shifting into the 8 position, this put ah in range of a class entry, causing any files to become encrypted and unaccessible afterwards.

[ghost]

After getting a few quiet minutes to acutally read the original code, now I see. The problem is a one line typo. You don't even need two lines to fix it. The second patch line.

mov ah, byte ptr ...

is all you need. Doing

xor ax, ax

will not cause any error, but it's unnecessary, and obscures what is intended. I would omit that, and patch only what is required. I've tested that, and it works for me.

Thanks for your hard work finding this bug.

[the-grue]

Have you tested it with security classes enabled? I am guessing there was a reason to zero out al in the first place. But you are probably correct, xor al,al would do the trick followed by the mov ah,...

[ghost]

No I've not tested it with classes. And yes there is a reason to zero out AL. But not AX, because as you see, the MOV to AH takes care of that.

If you study the 9 lines long enough, you will see why they zero out AL first

[ghost]

If we're going to create pull requests and merge patches before they've even been reviewed, I can't support that. If this is not fixed as outlined above, I'll move along.

[roelandjansen]

review to me looked good, if a pr had not been fully tested, it should imho not be submitted and kept in the test branch?

[ghost]

As I said above,

xor ax, ax

should not be in the patch

I explained why above. Is it not clear? I can try again if you're willing to fix it.

[roelandjansen]

maybe he has a reason to clear carry as well. If not needed, it can be removed with an additional pr.

note that I am on a holiday trip so that the capabilities are limited here.

[ghost]

xor al, al xor ax, ax

both clear the carry flag.

Thus, xor al, al in the original code does not need to be replaced, and should not be replaced. Patches should be the minimum required to fix the bug. Further changes should be in a separate patch. Which in this case are superfluous and only confuse the intent of the code.

[the-grue]

xor ax, ax

will not cause error, but it's unnecessary, and obscures what is intended. I would omit that, and patch only what is required, the MOV line

I will submit a fix to revert it back to xor al,al. It is an unnecessary change that shouldn't have made it into the patch.

It was just a typo.

As a lot of bugs are. In this case, a 1 character typo in over 100,000 lines of kernel code that brought a commercial product to its end.

I would omit the remark about encryption. I have a test file created under DOS, booted under MOS, its not encrypted, still fails to read with the unpatched code.

I disagree with this assessment. The reason you cannot read it is because with a class identified, it is assuming the data IS encrypted when it tries to read it back.

[the-grue]

See PR #52 to revert unnecessary change.