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

------------------------------------------------------------------------------