Problem report #85

Problem report #85

[Dan Nicolaescu]

CID: 85
Checker: USE_AFTER_FREE (help)
File: base/src/emacs/src/fileio.c
Function: Finsert_file_contents
Description: Using freed pointer "conversion_buffer"

Event freed_arg: Pointer "conversion_buffer" freed by function "xfree" [model]
Also see events: [double_free][double_free][use_after_free][use_after_free]

4326 		  xfree (conversion_buffer);
4327 		  coding_free_composition_data (&coding);

At conditional (1): "how_much == -1" taking false path

4328 		  if (how_much == -1)
4329 		    error ("IO error reading %s: %s",
4330 			   SDATA (orig_filename), emacs_strerror (errno));

At conditional (2): "how_much == -2" taking false path

4331 		  else if (how_much == -2)
4332 		    error ("maximum buffer size exceeded");
4333 		}
4334 	
4335 	      /* Compare the beginning of the converted file
4336 		 with the buffer text.  */
4337 	
4338 	      bufpos = 0;

Event use_after_free: Using freed pointer "conversion_buffer"
Also see events: [freed_arg][double_free][double_free][use_after_free]
At conditional (3): "bufpos < inserted" taking false path

4339 	      while (bufpos < inserted && same_at_start < same_at_end
4340 		     && FETCH_BYTE (same_at_start) == conversion_buffer[bufpos])
4341 		same_at_start++, bufpos++;
4342 	
4343 	      /* If the file matches the buffer completely,
4344 		 there's no need to replace anything.  */
4345 	

At conditional (4): "bufpos == inserted" taking true path

4346 	      if (bufpos == inserted)
4347 		{

Event double_free: Double free of pointer "conversion_buffer" in call to "xfree" [model]
Also see events: [freed_arg][double_free][use_after_free][use_after_free]

4348 		  xfree (conversion_buffer);
4349 		  coding_free_composition_data (&coding);
4350 		  emacs_close (fd);
4351 		  specpdl_ptr--;
4352 		  /* Truncate the buffer to the size of the file.  */
4353 		  del_range_byte (same_at_start, same_at_end, 0);
4354 		  inserted = 0;
4355 		  goto handled;
4356 		}
4357 	
4358 	      /* Extend the start of non-matching text area to multibyte
4359 		 character boundary.  */
4360 	      if (! NILP (current_buffer->enable_multibyte_characters))
4361 		while (same_at_start > BEGV_BYTE
4362 		       && ! CHAR_HEAD_P (FETCH_BYTE (same_at_start)))
4363 		  same_at_start--;
4364 	
4365 	      /* Scan this bufferful from the end, comparing with
4366 		 the Emacs buffer.  */
4367 	      bufpos = inserted;
4368 	
4369 	      /* Compare with same_at_start to avoid counting some buffer text
4370 		 as matching both at the file's beginning and at the end.  */

Event use_after_free: Using freed pointer "conversion_buffer"
Also see events: [freed_arg][double_free][double_free][use_after_free]

4371 	      while (bufpos > 0 && same_at_end > same_at_start
4372 		     && FETCH_BYTE (same_at_end - 1) == conversion_buffer[bufpos - 1])
4373 		same_at_end--, bufpos--;
4374 	
4375 	      /* Extend the end of non-matching text area to multibyte
4376 		 character boundary.  */
4377 	      if (! NILP (current_buffer->enable_multibyte_characters))
4378 		while (same_at_end < ZV_BYTE
4379 		       && ! CHAR_HEAD_P (FETCH_BYTE (same_at_end)))
4380 		  same_at_end++;
4381 	
4382 	      /* Don't try to reuse the same piece of text twice.  */
4383 	      overlap = same_at_start - BEGV_BYTE - (same_at_end + inserted - ZV_BYTE);
4384 	      if (overlap > 0)
4385 		same_at_end += overlap;
4386 	
4387 	      /* If display currently starts at beginning of line,
4388 		 keep it that way.  */
4389 	      if (XBUFFER (XWINDOW (selected_window)->buffer) == current_buffer)
4390 		XWINDOW (selected_window)->start_at_line_beg = Fbolp ();
4391 	
4392 	      /* Replace the chars that we need to replace,
4393 		 and update INSERTED to equal the number of bytes
4394 		 we are taking from the file.  */
4395 	      inserted -= (ZV_BYTE - same_at_end) + (same_at_start - BEGV_BYTE);
4396 	
4397 	      if (same_at_end != same_at_start)
4398 		{
4399 		  del_range_byte (same_at_start, same_at_end, 0);
4400 		  temp = GPT;
4401 		  same_at_start = GPT_BYTE;
4402 		}
4403 	      else
4404 		{
4405 		  temp = BYTE_TO_CHAR (same_at_start);
4406 		}
4407 	      /* Insert from the file at the proper position.  */
4408 	      SET_PT_BOTH (temp, same_at_start);
4409 	      insert_1 (conversion_buffer + same_at_start - BEGV_BYTE, inserted,
4410 			0, 0, 0);
4411 	      if (coding.cmp_data && coding.cmp_data->used)
4412 		coding_restore_composition (&coding, Fcurrent_buffer ());
4413 	      coding_free_composition_data (&coding);
4414 	
4415 	      /* Set `inserted' to the number of inserted characters.  */
4416 	      inserted = PT - temp;
4417 	      /* Set point before the inserted characters.  */
4418 	      SET_PT_BOTH (temp, same_at_start);
4419 	

Event double_free: Double free of pointer "conversion_buffer" in call to "xfree" [model]
Also see events: [freed_arg][double_free][use_after_free][use_after_free]

4420 	      xfree (conversion_buffer);
4421 	      emacs_close (fd);

Re: Problem report #85 RESOLVED

[Kenichi Handa]

In article <200605110120.k4B1Kjfj011062@scanner2.ics.uci.edu>, Dan Nicolaescu <dann@ics.uci.edu> writes:

> CID: 85
> Checker: USE_AFTER_FREE (help)
> File: base/src/emacs/src/fileio.c
> Function: Finsert_file_contents
> Description: Using freed pointer "conversion_buffer"

> Event freed_arg: Pointer "conversion_buffer" freed by function "xfree" [model]
> Also see events: [double_free][double_free][use_after_free][use_after_free]

> 4326 		  xfree (conversion_buffer);
> 4327 		  coding_free_composition_data (&coding);

> At conditional (1): "how_much == -1" taking false path

> 4328 		  if (how_much == -1)
> 4329 		    error ("IO error reading %s: %s",
> 4330 			   SDATA (orig_filename), emacs_strerror (errno));

> At conditional (2): "how_much == -2" taking false path

> 4331 		  else if (how_much == -2)
> 4332 		    error ("maximum buffer size exceeded");
> 4333 		}

This part of the code is surely suspicious.  Now the
relevant code is this:

      /* At this point, INSERTED is how many characters (i.e. bytes)
	 are present in CONVERSION_BUFFER.
	 HOW_MUCH should equal TOTAL,
	 or should be <= 0 if we couldn't read the file.  */

      if (how_much < 0)
	{
	  xfree (conversion_buffer);
	  coding_free_composition_data (&coding);
	  if (how_much == -1)
	    error ("IO error reading %s: %s",
		   SDATA (orig_filename), emacs_strerror (errno));
	  else if (how_much == -2)
	    error ("maximum buffer size exceeded");
	}

I think we must always signal an error if how_mach < 0.
And, I see no code setting how_much to -2 before there.
how_mach is set to negative only if emacs_read returned a
negative value.  In addition, we don't have to check
exceeding of buffer size here (it's done in insert_1) later.
So, I've just installed this change.

2006-05-11  Kenichi Handa  <handa@m17n.org>

	* fileio.c (Finsert_file_contents): Fix for the case of IO error
	while handling replace operation.

*** fileio.c	08 May 2006 13:13:09 +0900	1.564
--- fileio.c	11 May 2006 13:07:04 +0900	
***************
*** 4325,4335 ****
  	{
  	  xfree (conversion_buffer);
  	  coding_free_composition_data (&coding);
! 	  if (how_much == -1)
! 	    error ("IO error reading %s: %s",
! 		   SDATA (orig_filename), emacs_strerror (errno));
! 	  else if (how_much == -2)
! 	    error ("maximum buffer size exceeded");
  	}
  
        /* Compare the beginning of the converted file
--- 4325,4332 ----
  	{
  	  xfree (conversion_buffer);
  	  coding_free_composition_data (&coding);
! 	  error ("IO error reading %s: %s",
! 		 SDATA (orig_filename), emacs_strerror (errno));
  	}
  
        /* Compare the beginning of the converted file

---
Kenichi Handa
handa@m17n.org

Re: Problem report #85

[Jan Djärv]

Dan Nicolaescu skrev:
> CID: 85
> Checker: USE_AFTER_FREE (help)
> File: base/src/emacs/src/fileio.c
> Function: Finsert_file_contents
> Description: Using freed pointer "conversion_buffer"
> 
> Event freed_arg: Pointer "conversion_buffer" freed by function "xfree" [model]
> Also see events: [double_free][double_free][use_after_free][use_after_free]
> 
> 4326 		  xfree (conversion_buffer);
> 4327 		  coding_free_composition_data (&coding);
> 
> At conditional (1): "how_much == -1" taking false path
> 
> 4328 		  if (how_much == -1)
> 4329 		    error ("IO error reading %s: %s",
> 4330 			   SDATA (orig_filename), emacs_strerror (errno));
> 
> At conditional (2): "how_much == -2" taking false path
> 
> 4331 		  else if (how_much == -2)
> 4332 		    error ("maximum buffer size exceeded");
> 4333 		}

If how_much < 0 (tested before the code snippet above), it must be -1, since 
that is what emacs_read may return.  So "how_much == -1" taking false path can 
not happen.  I don't see how how_much can become -2, so that code is probably 
dead (but harmless).

	Jan D.