Undefined behavior in Figure 36.6 (Simplified xv6 IDE Disk Driver)

[BastiDood]

Chapter 36 (on I/O devices) provides a simplified implementation of xv6's IDE disk driver. Among the helper functions is ide_wait_ready, which (according to its function definition) returns an int. In the actual xv6 code, this int is used as an error indicator.

// Wait for IDE disk to become ready.
static int
idewait(int checkerr)
{
  int r;

  while(((r = inb(0x1f7)) & (IDE_BSY|IDE_DRDY)) != IDE_DRDY)
    ;
  if(checkerr && (r & (IDE_DF|IDE_ERR)) != 0)
    return -1;
  return 0;
}

However, in the simplified code (see Figure 36.6), observe the absence of the return statement. In C, it is undefined behavior for a value-returning function not to return anything (in any code path).

// Function signature says that we should return an `int`...
static int ide_wait_ready() {
  while (((int r = inb(0x1f7)) & IDE_BSY) || !(r & IDE_DRDY))
    ; // loop until drive isn’t busy

  // ... but we don't return anything! This is UB!
  // We probably meant to insert the line below for brevity...

  // return 0;
}

The actual invocation of undefined behavior occurs later in ide_intr, where we use the (presumed) return value of ide_wait_ready inside an if-condition.

void ide_intr() {
  struct buf *b;
  acquire(&ide_lock);

  // Undefined behavior here! Recall that we expect
  // `ide_wait_ready` to return an integer.
  if (!(b->flags & B_DIRTY) && ide_wait_ready() >= 0)
    insl(0x1f0, b->data, 512/4); // if READ: get data

  b->flags |= B_VALID;
  b->flags &= ˜B_DIRTY;
  wakeup(b);                       // wake waiting process
  if ((ide_queue = b->qnext) != 0) // start next request
    ide_start_request(ide_queue);  // (if one exists)
  release(&ide_lock);
}

[remzi-arpacidusseau]

fair point. I added one line to indicate the function should return something on success/error. Thanks! Your update will propagate to the ostep site and printed book soon, and your name will also soon appear in the preface. Much appreciated!

[BastiDood]

Awesome! Thank you so much. For your reference, I prefer to be named as "Sebastian Luis Ortiz" (affiliated with the University of the Philippines - Diliman) in the credits.