USF-OS / FogOS

Other
1 stars 24 forks source link

added sprintf() and snprintf() #39

Open amayaling opened 4 months ago

Jsk07211 commented 4 months ago

Emmanuel and I (Jaz) will do a code review on this :D

Jsk07211 commented 3 months ago

Sprintf

  1. Int not getting formatted into the buffer
    • int is written into buffer, but buffer pointer is not incremented, hence null terminator is placed before the int
      • Possible fix: Have sprint_printint return the length of int written, which is not required for printint since file keeps track of where cursor is, but buffer does not
        offset = sprint_printint(buffer, va_arg(ap, int), 10, 1);
        buffer += offset;
  2. Formatting for %x is missing
    • Possible fix: Insert prefix before getting intval
      if (c == 'x') {
      *buffer++ = '0';
      *buffer++ = 'x';
      offset = sprint_printint(buffer, va_arg(ap, int), 16, 0);
      buffer += offset;
      }
  3. Works well for strings and longs :^)

Snprintf

  1. Length checking is only done in outside for loop, but values are directly added to buffer in sprint_printint/sprintptr, which can cause size of buffer to be exceeded (not memory safe)

    • Possible Fix 1: Send a test buffer to be filled and check if strlen(test_buf) + offset >= size, else copy contents of test_buf into buffer if it is safe (Make sure there's space for null termination~~)
    • Possible Fix 2: Add length restrictions to sprint_printint/sprintptr (although it may cause problems for sprintf stuff?)
  2. Similar comments to sprintf

Test Cases Ran

#define BUF_SZ 16

int
main(void)
{
  char buf[BUF_SZ];
  char str[BUF_SZ] = "STRING";
  int a = 15, b = 25, sum;
  sum = a + b;

  // sprintf %s 
  sprintf(buf, "s: %s\n", str);
  printf("%s\n", buf);

  memset(buf, 0, BUF_SZ);

  // sprintf %d
  sprintf(buf, "%d + %d = %d\n", a, b, sum);
  printf("%s\n", buf);

  memset(buf, 0, BUF_SZ);

  // snprintf %d (with buffer overflow)
  snprintf(buf, "The sum of %d and %d is the value: %d\n", a, b, sum);
  printf("%s\n", buf);

  memset(buf, 0, BUF_SZ);

  return 0;
}

Overall

Requires a few small fixes and the code should be good to go :D

malensek commented 2 months ago

The suggested fixes above are good and should be implemented. Additionally, there is some code redundancy here: for example, the two printints are almost the same and could be condensed into a single function (with a second helper function).