Subject: Bug fix for "disklabel.c"
To: None <netbsd-bugs@sun-lamp.cs.berkeley.edu>
From: Shao Ai Wu <m-sw2360@HAPPY.CS.NYU.EDU>
List: netbsd-bugs
Date: 01/20/1994 03:42:26
>>> Well see for yourself : Here is the code asking you if you want to overwr. dos-parts :
>>> if (dosdp && dosdp->dp_typ == DOSPTYP_386BSD && pp->p_size &&
>>> dosdp->dp_start == pp->p_offset) {
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> lbl_off = pp->p_offset;
>>> } else {
>>> if (dosdp) {
>>> char c;
>>>
>>> printf("overwriting disk with DOS partition table? (y):");
>>> fflush(stdout);
>>> c = getchar();
>>> if (c != EOF && c != (int)'\n')
>>> while (getchar() != (int)'\n')
>>> ;
>>> if (c == (int)'n')
>>> exit(0);
>>> }
>>> lbl_off = 0;
>>> So the only reason when this happens is when offsets aren't matching.
>>> B.Wiserner
Thank you very much for quoting the actual source code.
I found the PROBLEMS!!
PROBLEM #1:
if (... && dosdp->dp_start == pd->p_offset) {
^^^
this equality comparison is too strong! It would be
much better and safer if it were "<=". (there is also
a problem with "<=" as describe later.)
For example: "==" would cause a partition table destroy:
suppose the user wants to put BSD partition as the last partition,
and keep a fee space in the midle while DOS is the first
partition. The intended hard disk layout is the following:
Cylinder 0: reservered for hard disk tables
cylinder 1 - 300: DOS primary
cylinder 301 - 600: intended for other use later
cylinder 601 - 900: BSD.
In this case, dosdp->dp_start == 301 and pd->p_offset == 601
The result of that code is DESTROYING the partition table!!
And this intention is perfectly possible and acceptable.
More importantly, the phisical cylinder is different from
the logical cylinder (the translated cylinder).
For example, (use my hard disk as an example)
Microplis 1598:
Physical: 15 heads, 1928 cylinders, 71 sectors per track.
Physical cylinder size: 15x71 sectors
Logical: 64 heads, 32 sectors/track, 991 cylinders
Logical cylinder size: 64x32 sectors
If the user wants to put BSD partition as the first
partition, and use the translated geomatry:
the input offset is one logical cylinder: 64x32 sectors
BUT
the actual free space starts at: 15x71
the only chance for this algorithm using "==" to work is
the DOS partition is the first partition, and *BSD follows
DOS immediately. Other than this, *BSD with this partition
scheme can't co-exist with other OS!!
The quick fix is to replace "==" by "<=".
However, the quick fix does not solve the long term problem; a more
error checking scheme is needed.
PROBLEM #2:
>>> if (dosdp && dosdp->dp_typ == DOSPTYP_386BSD && pp->p_size &&
>>> dosdp->dp_start == pp->p_offset) {
>>> lbl_off = pp->p_offset;
>>> } else {
>>> ...
>>> lbl_off = 0; <<<<<<<<<<< ***** WHY??? ***********
>>> }
This simple assigment kills the entire hard disk!!!
A complete solution is as the following: (patch for "disklabel.c")
if (dosdp && dosdp->dp_typ == DOSPTYP_386BSD && pp->p_size &&
dosdp->dp_start <= pp->p_offset) {
if (dosdp->dp_size >= pp->p_size + (pp->p_offset - dosdp->dp_start))
lbl_off = pp->p_offset;
else {
fprintf (stderr, "Insurficient free disk space.\n");
exit (1);
/* there is a problem: free space is smaller than needed */
}
} else {
if (dosdp && dosdp->dp_typ == DOSPTYP_386BSD && pp->p_size &&
dosdp->dp_size >= pp->p_size + (pp->p_offset - dosdp->dp_start))
lbl_off = dosdp->dp_start; /* ok use the free space */
else {
/* ask if the user wants to kill all partitions */
/* if yes then */
/* lbl_off = 0; */
/* else { */
/* fprintf (stderr, "Invalid hard partition.\n"); */
/* exit (1); */
/* } */
}
}
DISCLAIM:
This quick patch is limited to my understanding to the source segment
quoted. I have not yet tested this code in my computer since it is too
painful to restore the current setup in my system, and I need the system
for full service at this time.
NOTE:
If this fix is correct, please post it to the Internet, and apply it to
the source code for next release of the *BSD.
Shao Ai Wu
m-sw2360@cs.nyu.edu
------------------------------------------------------------------------------