Re: [RFA v2 03/24] Change return type of find_and_open_script

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Hi Tom,

I like the change, but one thing caught my attention.

On 2017-07-25 19:20, Tom Tromey wrote:
> @@ -953,26 +950,25 @@ source_script_file (struct auto_load_pspace_info
> *pspace_info,
>        return;
>      }
>
> -  opened = find_and_open_script (file, 1 /*search_path*/,
> -				 &stream, &full_path);
> +  gdb::optional<open_script> opened = find_and_open_script (file,
> +							    1 /*search_path*/);
> +  const char *path_ptr;
>
> -  cleanups = make_cleanup (null_cleanup, NULL);
>    if (opened)
>      {
> -      make_cleanup_fclose (stream);
> -      make_cleanup (xfree, full_path);
> -
> -      if (!file_is_auto_load_safe (full_path,
> +      path_ptr = opened->full_path.get ();
> +      if (!file_is_auto_load_safe (opened->full_path.get (),
>  				   _("auto-load: Loading %s script "
>  				     "\"%s\" from section \"%s\" of "
>  				     "objfile \"%s\".\n"),
> -				   ext_lang_name (language), full_path,
> +				   ext_lang_name (language),
> +				   opened->full_path.get (),
>  				   section_name, objfile_name (objfile)))
> -	opened = 0;
> +	opened.reset ();
>      }
>    else
>      {
> -      full_path = NULL;
> +      path_ptr = NULL;
>
>        /* If one script isn't found it's not uncommon for more to not 
> be
>  	 found either.  We don't want to print a message for each script,
> @@ -986,14 +982,12 @@ source_script_file (struct auto_load_pspace_info
> *pspace_info,
>  					    section_name, offset);
>      }
>
> -  in_hash_table = maybe_add_script_file (pspace_info, opened, file, 
> full_path,
> -					 language);
> +  in_hash_table = maybe_add_script_file (pspace_info, bool (opened), 
> file,
> +					 path_ptr, language);

Here, isn't path_ptr used after free in case the auto-load was denied?

>    /* If this file is not currently loaded, load it.  */
>    if (opened && !in_hash_table)
> -    sourcer (language, objfile, stream, full_path);
> -
> -  do_cleanups (cleanups);
> +    sourcer (language, objfile, opened->stream.get (), path_ptr);
>  }
>
>  /* Subroutine of source_section_scripts to simplify it.

Thanks,

Simon