Re: frysk-sys/frysk bindir/ChangeLog proc/live/Cha ...

Re: frysk-sys/frysk bindir/ChangeLog proc/live/Cha ...

[Mark Wielaard]

Hi Andrew,

On Wed, 2007-07-18 at 19:11 +0000, cagney@sourceware.org wrote: 
> 	2007-07-18  Andrew Cagney  <cagney@redhat.com>
> 	
> 	* LinuxProc.java: Use frysk.sys.proc.Exe.get.  Validate the
> 	returned string, returning NULL when a problem.

>  /**
>   * A Linux Proc tracked using PTRACE.
> @@ -199,19 +199,32 @@
>      }
>      /**
>       * Get the Executable.
> -     */
> -    protected String sendrecExe ()
> -    {
> -      String exeString = "/proc/" + getPid() + "/exe";
> -      try
> -	{
> -	  exeString = new File(exeString).getCanonicalPath();
> -	}
> -      catch (IOException ioe)
> -	{
> -	  // Just return exeString. No permission or process died.
> -	}
> -      return exeString;
> +     *
> +     * XXX: This is racy - it can miss file renames.  The alternative
> +     * would be to have two methods; one returning a file descriptor
> +     * and a second returning the exe as it should be (but possibly
> +     * isn't :-).  Better yet have utrace handle it :-)
> +     */
> +    protected String sendrecExe () {
> +	String exe = Exe.get(getPid());
> +	// Linux's /proc/$$/exe can get screwed up in several ways.
> +	// Detect each here and return null.
> +	if (exe.endsWith(" (deleted)"))
> +	    // Assume (possibly incorrectly) that a trailing
> +	    // "(deleted)" always indicates a deleted file.
> +	    return null;
> +	if (exe.indexOf((char)0) >= 0)
> +	    // Assume that an EXE that has somehow ended up with an
> +	    // embedded NUL character is invalid.  This happens when
> +	    // the kernel screws up "mv a-really-long-file $exe"
> +	    // leaving the updated EXE string with something like
> +	    // "$exe<NUL>ally-long-file (deleted)".
> +	    return null;
> +	if (new File(exe).exists())
> +	    // Final sanity check; the above two should have covered
> +	    // all possible cases.  But one never knows.
> +	    return exe;
> +	return null;
>      }

This break FryskGui which isn't prepared to handle null from Proc.get().

It is probably a good idea to make Proc.get() return a File object that
the user can then inspect themselves (you won't get rid of the race
anyway unless you explicitly open the file and return that as result,
but that is probably not what you want here), then you can also easily
find all users of this method so you can inspect whether or not they
handle the new return value correctly.

        2007-07-18  Andrew Cagney  <cagney@redhat.com>
        
        * cni/Exe.cxx (get): Allow space for "(deleted)").

If you do make getExe() return File, then you can also just use the
standard File.getCanonicalFile() method, which will do all sanity
checking already in proc.Exe so we don't need a special case cni
implementation for it and doesn't depend on fixed buffer lengths.

Cheers,

Mark

[patch] PickProcDialog vs Proc.getExe()

[Mark Wielaard]

Hi,

On Wed, 2007-07-25 at 13:53 +0200, Mark Wielaard wrote:
> This break FryskGui which isn't prepared to handle null from Proc.get().

Here is a workaround for at least one case (there might be others, but I
haven't looked too deeply yet). This gets FryskGui up and running again
for me.

2007-07-25  Mark Wielaard  <mwielaard@redhat.com>

    * PickProcDialog.java (ProcCreatedObserver.run): Check whether
    Proc.getExe() returns null before constructing File path.

Cheers,

Mark

diff -u -r1.12 PickProcDialog.java
--- frysk-gui/frysk/gui/monitor/PickProcDialog.java     30 Jan 2007 00:18:21 -0000      1.12
+++ frysk-gui/frysk/gui/monitor/PickProcDialog.java     25 Jul 2007 11:55:53 -0000
@@ -355,11 +355,14 @@
             {
               iterMap.put(proc.getCommand(), process);
               treeStore.setValue(process, nameDC, proc.getCommand());
-              File path = new File(proc.getExe());
-              
-              if (path != null)
-                treeStore.setValue(process, locationDC,
-                                   justPath(path.getPath(), path.getName()));
+
+             String exe = proc.getExe();
+             if (exe != null)
+               {
+                 File path = new File(exe);
+                 treeStore.setValue(process, locationDC,
+                                    justPath(path.getPath(), path.getName()));
+               }
               else
                 treeStore.setValue(process, locationDC, "");