9552 grep segfaults when you ask for context

Review Request #1090 — Created May 24, 2018 and submitted

andy_js
illumos-gate
9552
general

You can specify the number of surrounding lines that should also be printed out for each match (with -B NUM, -C NUM or just -NUM) as context, but if you do grep with core dump. This is because the check that tests whether the buffer needs to grow is faulty. Due to missing parenthesis it causes the buffer to balloon prematurely and then later not grow when the buffer is no longer big enough, causing a buffer overflow. It's also possible that after growing it still won't be big enough so we need to do the check in a loop.

The stack trace wasn't much good (it was blowing up in memcpy) so I inserted a bunch of print statements to figure out what was wrong and then fixed the code. No segfaults anymore!

  • 0
  • 0
  • 0
  • 2
  • 2
Description From Last Updated
yuripv
  1. 
      
  2. usr/src/cmd/grep/grep.c (Diff revision 1)
     
     

    Hmmm, can we move this out of the loop so that we do realloc with needed size just 1 time?

    1. While it's not ideal I don't think it's a big deal either. It should not happen very often (you would need a line longer than 1000 characters to trigger it).

    2. Now fixed.

  3. 
      
yuripv
  1. Ship It!
  2. 
      
tsoome
  1. Ship It!
  2. 
      
citrus
  1. Ship It!
  2. 
      
mbarden
  1. Thanks for fixing this.

    Rather than do a while() loop allocating BUFSIZE each time, we can just calculate how many multiples of BUFSIZE we need. Something like (((line_len + 1) / BUFSIZE) + 1), rather than just BUFSIZE.

  2. 
      
andy_js
mbarden
  1. 
      
  2. usr/src/cmd/grep/grep.c (Diff revision 2)
     
     

    We're trying to make (line_len + 1) room in the buffer, in multiples of BUFSIZE. if line_len is exactly BUFSIZE, then we need to allocate at least BUFSIZE+1 bytes. However, this expression will only evaluate to BUFSIZE bytes. While we'll allocate BUFSIZE+1 bytes because we add an extra byte in the realloc expression, conbuflen itself won't represent that final byte (which, unlike the extra byte in realloc, is something actually contained in the 'context buffer').
    Should we drop the -1, out of an abundance of caution? At least, a comment here explaining would help.

    1. I just noticed that the check on 1287 also needs to be updated. Perhaps I should punt on this part of the change and instead focus on fixing the segfault. What do you reckon?

    2. I thought this was part of the segfault?

    3. No it's the extra parenthesis which fix the segfault.

    4. If we can get a simple fix here, it would be nice it as part of this fix, but if it ends up being more involved, I don't mind it if you open a new bug just for the allocation problem, just so it doesn't block this fix.

    5. I think I'm going to go ahead with the while-loop fix. I already have enough reviews for that.

  3. 
      
andy_js
kmays
  1. Ship It!
  2. 
      
andy_js
Review request changed

Status: Closed (submitted)

Loading...