[PATCH-libreport] report-python: don't include executable

[Martin Milata]

Doesn't really make sense, as it is always path to the python
executable. bz#741255
---
 src/include/problem_data.h       |    3 +++
 src/lib/problem_data.c           |    5 +++++
 src/report-python/problem_data.c |    8 ++++++++
 3 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/src/include/problem_data.h b/src/include/problem_data.h
index 14d1240..87037a2 100644
--- a/src/include/problem_data.h
+++ b/src/include/problem_data.h
@@ -74,6 +74,9 @@ void add_to_problem_data(problem_data_t *problem_data,
                 const char *name,
                 const char *content);
 
+/* Removes item from problem data */
+void remove_from_problem_data(problem_data_t *problem_data, const char *key);
+
 static inline struct problem_item *get_problem_data_item_or_NULL(problem_data_t
*problem_data, const char *key)
 {
     return (struct problem_item *)g_hash_table_lookup(problem_data, key);
diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c
index 2ddff86..7d85709 100644
--- a/src/lib/problem_data.c
+++ b/src/lib/problem_data.c
@@ -155,6 +155,11 @@ void add_to_problem_data(problem_data_t *problem_data,
     add_to_problem_data_ext(problem_data, name, content, CD_FLAG_TXT +
CD_FLAG_ISNOTEDITABLE);
 }
 
+void remove_from_problem_data(problem_data_t *problem_data, const char *key)
+{
+    g_hash_table_remove(problem_data, key);
+}
+
 const char *get_problem_item_content_or_die(problem_data_t *problem_data, const char
*key)
 {
     struct problem_item *item = get_problem_data_item_or_NULL(problem_data, key);
diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c
index 9067dea..8b4088d 100644
--- a/src/report-python/problem_data.c
+++ b/src/report-python/problem_data.c
@@ -115,8 +115,16 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject *pself,
PyObject *
 static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null)
 {
     p_problem_data *self = (p_problem_data*)pself;
+    int have_executable = (get_problem_data_item_or_NULL(self->cd,
"executable") != NULL);
+
     add_basics_to_problem_data(self->cd);
 
+    /* Workaround - the executable item will always be /usr/bin/python, which is useless.
*/
+    if (!have_executable)
+    {
+        remove_from_problem_data(self->cd, "executable");
+    }
+
     Py_RETURN_NONE;
 }
 
-- 
1.7.6.4

[Vratislav Podzimek]

On Thu, 2011-10-20 at 12:31 +0200, Martin Milata wrote:
>  Doesn't really make sense, as it is always path to the python
>  executable. bz#741255 
I cannot agree with you. As I see the purpose of the
"executable:
SOMETHING" item in the bugreport it's used to differentiate between
running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to
the /usr/bin/vim). According to the value of argv[0] vim decides how it
should run. And you can do exactly the same in Python and sys.argv[0].

And even if this item should have been removed this wouldn't have been
the right way to do it -- let handlers query and pass this info and then
remove it somewhere else?

If you look at the https://bugzilla.redhat.com/show_bug.cgi?id=747562
which is a testing bugreport for my python-meh and libreport patches
"executable: /usr/sbin/firstboot" is redundant here. And I think the
righ way how to report this info is to use "cmdline: SOMETHING"
containing (in case of python script):
"cmdline: PATH_TO_PYTHON PYTHON_OPTIONS EXECUTABLE EXECUTABLE_OPTIONS"

for example: "cmdline: /usr/bin/python -t -Q /usr/bin/phatch
halved.phatch"

Which gives much more information (e.g. python 2 vs. python 3?) and
includes also the "executable". 

I don't think it would resolve bz#741255. But this should be solved
somewhere else.

--
Vratislav Podzimek

>  ---
>   src/include/problem_data.h       |    3 +++
>   src/lib/problem_data.c           |    5 +++++
>   src/report-python/problem_data.c |    8 ++++++++
>   3 files changed, 16 insertions(+), 0 deletions(-)
>  
>  diff --git a/src/include/problem_data.h b/src/include/problem_data.h
>  index 14d1240..87037a2 100644
>  --- a/src/include/problem_data.h
>  +++ b/src/include/problem_data.h
>  @@ -74,6 +74,9 @@ void add_to_problem_data(problem_data_t *problem_data,
>                   const char *name,
>                   const char *content);
>   
>  +/* Removes item from problem data */
>  +void remove_from_problem_data(problem_data_t *problem_data, const char *key);
>  +
>   static inline struct problem_item *get_problem_data_item_or_NULL(problem_data_t
> *problem_data, const char *key)
>   {
>       return (struct problem_item *)g_hash_table_lookup(problem_data, key);
>  diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c
>  index 2ddff86..7d85709 100644
>  --- a/src/lib/problem_data.c
>  +++ b/src/lib/problem_data.c
>  @@ -155,6 +155,11 @@ void add_to_problem_data(problem_data_t *problem_data,
>       add_to_problem_data_ext(problem_data, name, content, CD_FLAG_TXT +
> CD_FLAG_ISNOTEDITABLE);
>   }
>   
>  +void remove_from_problem_data(problem_data_t *problem_data, const char *key)
>  +{
>  +    g_hash_table_remove(problem_data, key);
>  +}
>  +
>   const char *get_problem_item_content_or_die(problem_data_t *problem_data, const char
> *key)
>   {
>       struct problem_item *item = get_problem_data_item_or_NULL(problem_data, key);
>  diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c
>  index 9067dea..8b4088d 100644
>  --- a/src/report-python/problem_data.c
>  +++ b/src/report-python/problem_data.c
>  @@ -115,8 +115,16 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject
> *pself, PyObject *
>   static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null)
>   {
>       p_problem_data *self = (p_problem_data*)pself;
>  +    int have_executable = (get_problem_data_item_or_NULL(self->cd,
> "executable") != NULL);
>  +
>       add_basics_to_problem_data(self->cd);
>   
>  +    /* Workaround - the executable item will always be /usr/bin/python, which is
> useless. */
>  +    if (!have_executable)
>  +    {
>  +        remove_from_problem_data(self->cd, "executable");
>  +    }
>  +
>       Py_RETURN_NONE;
>   }
>

[Michal Toman]

On 21.10.2011 12:16, Vratislav Podzimek wrote:
>  I cannot agree with you. As I see the purpose of the
> "executable:
>  SOMETHING" item in the bugreport it's used to differentiate between
>  running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to
>  the /usr/bin/vim). According to the value of argv[0] vim decides how it
>  should run. And you can do exactly the same in Python and sys.argv[0]. 
Two different use-cases (Python exception vs. SELinux denial). Sealert 
always sets executable to python which is not helpful at all.

On 20.10.2011 12:31, Martin Milata wrote:
>  Doesn't really make sense, as it is always path to the python
>  executable. bz#741255
>  ---
>   src/include/problem_data.h       |    3 +++
>   src/lib/problem_data.c           |    5 +++++
>   src/report-python/problem_data.c |    8 ++++++++
>   3 files changed, 16 insertions(+), 0 deletions(-)
>
>  diff --git a/src/include/problem_data.h b/src/include/problem_data.h
>  index 14d1240..87037a2 100644
>  --- a/src/include/problem_data.h
>  +++ b/src/include/problem_data.h
>  @@ -74,6 +74,9 @@ void add_to_problem_data(problem_data_t *problem_data,
>                   const char *name,
>                   const char *content);
>
>  +/* Removes item from problem data */
>  +void remove_from_problem_data(problem_data_t *problem_data, const char *key);
>  +
>   static inline struct problem_item *get_problem_data_item_or_NULL(problem_data_t
> *problem_data, const char *key)
>   {
>       return (struct problem_item *)g_hash_table_lookup(problem_data, key);
>  diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c
>  index 2ddff86..7d85709 100644
>  --- a/src/lib/problem_data.c
>  +++ b/src/lib/problem_data.c
>  @@ -155,6 +155,11 @@ void add_to_problem_data(problem_data_t *problem_data,
>       add_to_problem_data_ext(problem_data, name, content, CD_FLAG_TXT +
> CD_FLAG_ISNOTEDITABLE);
>   }
>
>  +void remove_from_problem_data(problem_data_t *problem_data, const char *key)
>  +{
>  +    g_hash_table_remove(problem_data, key);
>  +}
>  +
>   const char *get_problem_item_content_or_die(problem_data_t *problem_data, const char
> *key)
>   {
>       struct problem_item *item = get_problem_data_item_or_NULL(problem_data, key);
>  diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c
>  index 9067dea..8b4088d 100644
>  --- a/src/report-python/problem_data.c
>  +++ b/src/report-python/problem_data.c
>  @@ -115,8 +115,16 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject
> *pself, PyObject *
>   static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null)
>   {
>       p_problem_data *self = (p_problem_data*)pself;
>  +    int have_executable = (get_problem_data_item_or_NULL(self->cd,
> "executable") != NULL);
>  +
>       add_basics_to_problem_data(self->cd);
>
>  +    /* Workaround - the executable item will always be /usr/bin/python, which is
> useless. */
>  +    if (!have_executable)
>  +    {
>  +        remove_from_problem_data(self->cd, "executable");
>  +    }
>  +
>       Py_RETURN_NONE;
>   } 
This *always* removes the executable field, when called from Python 
binding. That will (at least) result into a different behavior of C and 
Python API.
If I understand correctly, what we want to do is to remove the 
executable (or executable == /usr/bin/python ?) from *SELinux* report, 
no matter if called from C or Python.

M.

[Martin Milata]

On Fri, Oct 21, 2011 at 12:50:17 +0200, Michal Toman wrote:
>  On 21.10.2011 12:16, Vratislav Podzimek wrote:
>  >I cannot agree with you. As I see the purpose of the "executable:
>  >SOMETHING" item in the bugreport it's used to differentiate between
>  >running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to
>  >the /usr/bin/vim). According to the value of argv[0] vim decides how it
>  >should run. And you can do exactly the same in Python and sys.argv[0].
>  
>  Two different use-cases (Python exception vs. SELinux denial).
>  Sealert always sets executable to python which is not helpful at
>  all. 
I see -- I overlooked the other use-case and assumed that the python
bindings are only used to report problems which have nothing to do with
current process (as is the case with Sealert).

>  On 20.10.2011 12:31, Martin Milata wrote:
>  >Doesn't really make sense, as it is always path to the python
>  >executable. bz#741255
>  >---
>  > [patch]
>  
>  This *always* removes the executable field, when called from Python
>  binding. That will (at least) result into a different behavior of C
>  and Python API.
>  If I understand correctly, what we want to do is to remove the
>  executable (or executable == /usr/bin/python ?) from *SELinux*
>  report, no matter if called from C or Python. 
I tried to solve it in a different way, patch should arrive as a reply
to this message.

Generally, I think that what we want to do is provide interfaces for
creating two kinds of reports -- reports about current process, where we
can detect things like executable, and reports about something else,
which is the case with sealert and where the caller should provide all
the information.

[PATCH-libreport] report-python: don't include executable for alerts

[Martin Milata]

Do not call add_basics() in createAlertSignature, which is probably only
used by Sealert. The method detects and adds executable item containing
path to the executable of the current process, which is useless as the
report is not about current process.
---
 src/report-python/__init__.py |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/report-python/__init__.py b/src/report-python/__init__.py
index 7955878..ac1da3a 100644
--- a/src/report-python/__init__.py
+++ b/src/report-python/__init__.py
@@ -137,7 +137,7 @@ def createAlertSignature(component, hashmarkername, hashvalue,
summary, alertSig
     pd.add("duphash", hashvalue)
     pd.add("reason", summary)
     pd.add("description", alertSignature)
-    pd.add_basics()
+    pd.add("analyzer", "libreport")
 
     return pd
 
-- 
1.7.6.4

[Vratislav Podzimek]

On Wed, 2011-10-26 at 12:47 +0200, Martin Milata wrote:
>  On Fri, Oct 21, 2011 at 12:50:17 +0200, Michal Toman wrote:
>  > On 21.10.2011 12:16, Vratislav Podzimek wrote:
>  > >I cannot agree with you. As I see the purpose of the "executable:
>  > >SOMETHING" item in the bugreport it's used to differentiate between
>  > >running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to
>  > >the /usr/bin/vim). According to the value of argv[0] vim decides how it
>  > >should run. And you can do exactly the same in Python and sys.argv[0].
>  > 
>  > Two different use-cases (Python exception vs. SELinux denial).
>  > Sealert always sets executable to python which is not helpful at
>  > all.
>  
>  I see -- I overlooked the other use-case and assumed that the python
>  bindings are only used to report problems which have nothing to do with
>  current process (as is the case with Sealert).
>  
>  > On 20.10.2011 12:31, Martin Milata wrote:
>  > >Doesn't really make sense, as it is always path to the python
>  > >executable. bz#741255
>  > >---
>  > > [patch]
>  > 
>  > This *always* removes the executable field, when called from Python
>  > binding. That will (at least) result into a different behavior of C
>  > and Python API.
>  > If I understand correctly, what we want to do is to remove the
>  > executable (or executable == /usr/bin/python ?) from *SELinux*
>  > report, no matter if called from C or Python.
>  
>  I tried to solve it in a different way, patch should arrive as a reply
>  to this message.
>  
>  Generally, I think that what we want to do is provide interfaces for
>  creating two kinds of reports -- reports about current process, where we
>  can detect things like executable, and reports about something else,
>  which is the case with sealert and where the caller should provide all
>  the information. 
I think that the key is not to override info provided from
"caller" (like in the attached patch) and patch "callers" to provide
it
correctly. It's the same for python-meh where I'am doing it right now.

--
Vratislav Podzimek

[PATCH-libreport] report-python: don't autodetect executable for sealert reports

[Martin Milata]

Here's another attempt at solving the bug. It adds a flag to
add_basics_to_problem_data indicating whether or not we are interested about
data from current process. Additionally, it adds optional argument to
createAlertSignature, which can be used by sealert to pass the executable
directly.

Suggestions are welcome.

Martin

-->8-->8-->8-->8-->8-->8-->8-->8-->8-->8-->8--

- add_basics_to_problem_data takes parameter indicating whether we are
  adding information about current process
- function for adding basic data not from the current process is added
  to the python api
- createAlertSignature takes optional argument that can be used to
  manually set the executable
---
 src/include/problem_data.h       |    4 +++-
 src/lib/problem_data.c           |    4 ++--
 src/report-python/__init__.py    |    6 ++++--
 src/report-python/problem_data.c |   19 ++++++++++++++-----
 4 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/src/include/problem_data.h b/src/include/problem_data.h
index 14d1240..f332433 100644
--- a/src/include/problem_data.h
+++ b/src/include/problem_data.h
@@ -57,7 +57,9 @@ typedef GHashTable problem_data_t;
 
 problem_data_t *new_problem_data(void);
 
-void add_basics_to_problem_data(problem_data_t *pd);
+/* If this_process is true, it means we are adding information about current
+ * process and can e.g. read path of the executable from /proc. */
+void add_basics_to_problem_data(problem_data_t *pd, int this_process);
 
 static inline void free_problem_data(problem_data_t *problem_data)
 {
diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c
index 9d45d3e..7f81ea3 100644
--- a/src/lib/problem_data.c
+++ b/src/lib/problem_data.c
@@ -57,7 +57,7 @@ problem_data_t *new_problem_data(void)
                  free, free_problem_item);
 }
 
-void add_basics_to_problem_data(problem_data_t *pd)
+void add_basics_to_problem_data(problem_data_t *pd, int this_process)
 {
     const char *analyzer = get_problem_item_content_or_NULL(pd, FILENAME_ANALYZER);
     if (analyzer == NULL)
@@ -106,7 +106,7 @@ void add_basics_to_problem_data(problem_data_t *pd)
 
     pid_t pid = getpid();
     const char *executable = get_problem_item_content_or_NULL(pd, FILENAME_EXECUTABLE);
-    if (executable == NULL && pid > 0)
+    if (executable == NULL && this_process && pid > 0)
     {
         char buf[PATH_MAX+1];
         char *exe = xasprintf("/proc/%u/exe", pid);
diff --git a/src/report-python/__init__.py b/src/report-python/__init__.py
index 6f4f543..a7f7dc3 100644
--- a/src/report-python/__init__.py
+++ b/src/report-python/__init__.py
@@ -130,14 +130,16 @@ def getVersion():
 
     return _hardcoded_default_version
 
-def createAlertSignature(component, hashmarkername, hashvalue, summary, alertSignature):
+def createAlertSignature(component, hashmarkername, hashvalue, summary, alertSignature,
executable=None):
     pd = problem_data()
     pd.add("component", component)
     pd.add("hashmarkername", hashmarkername)
     pd.add("duphash", hashvalue)
     pd.add("reason", summary)
     pd.add("description", alertSignature)
-    pd.add_basics()
+    if executable:
+        pd.add("executable", executable)
+    pd.add_basics_other_process()
 
     return pd
 
diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c
index 9067dea..791c439 100644
--- a/src/report-python/problem_data.c
+++ b/src/report-python/problem_data.c
@@ -115,7 +115,15 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject *pself,
PyObject *
 static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null)
 {
     p_problem_data *self = (p_problem_data*)pself;
-    add_basics_to_problem_data(self->cd);
+    add_basics_to_problem_data(self->cd, 1);
+
+    Py_RETURN_NONE;
+}
+
+static PyObject *p_add_basics_to_problem_data_other_proc(PyObject *pself, PyObject
*always_null)
+{
+    p_problem_data *self = (p_problem_data*)pself;
+    add_basics_to_problem_data(self->cd, 0);
 
     Py_RETURN_NONE;
 }
@@ -127,10 +135,11 @@ static PyObject *p_add_basics_to_problem_data(PyObject *pself,
PyObject *always_
 
 static PyMethodDef p_problem_data_methods[] = {
     /* method_name, func, flags, doc_string */
-    { "add"            , p_problem_data_add                 , METH_VARARGS },
-    { "get"            , p_get_problem_data_item            , METH_VARARGS },
-    { "create_dump_dir", p_create_dump_dir_from_problem_data, METH_VARARGS },
-    { "add_basics",      p_add_basics_to_problem_data,        METH_NOARGS },
+    { "add"                     , p_problem_data_add                     ,
METH_VARARGS },
+    { "get"                     , p_get_problem_data_item                ,
METH_VARARGS },
+    { "create_dump_dir"         , p_create_dump_dir_from_problem_data    ,
METH_VARARGS },
+    { "add_basics"              , p_add_basics_to_problem_data           ,
METH_NOARGS },
+    { "add_basics_other_process", p_add_basics_to_problem_data_other_proc,
METH_NOARGS },
     { NULL }
 };
 
-- 
1.7.6.4

[PATCH-libreport] report-python: don't autodetect executable for sealert reports

[Nikola Pajkovsky]

>  Here's another attempt at solving the bug. It adds a flag to
>  add_basics_to_problem_data indicating whether or not we are
>  interested about
>  data from current process. Additionally, it adds optional argument to
>  createAlertSignature, which can be used by sealert to pass the
>  executable
>  directly.
>   
ok, patch seems good. what I would like to see is only addition of an optional
argument to *createAlertSignature* and let setroubleshoot and anaconda fix this issue.

[PATCH-libreport] report-python: don't autodetect executable for sealert reports

[Jiri Moskovcak]

On 03/19/2012 11:58 AM, Nikola Pajkovsky wrote:
> > Here's another attempt at solving the bug. It adds a flag to
> > add_basics_to_problem_data indicating whether or not we are
> > interested about
> > data from current process. Additionally, it adds optional argument to
> > createAlertSignature, which can be used by sealert to pass the
> > executable
> > directly.
> >
>
>  ok, patch seems good. what I would like to see is only addition of an optional
>  argument to *createAlertSignature* and let setroubleshoot and anaconda fix this issue.
- Vrata is working on a patch for python-meh and abrt-server to do 
exactly this..

Vrato, what's the news?

Thanks,
Jirka

[PATCH-libreport] report-python: don't autodetect executable for sealert reports

[Vratislav Podzimek]

On Mon, 2012-03-19 at 12:49 +0100, Jiri Moskovcak wrote:
>  On 03/19/2012 11:58 AM, Nikola Pajkovsky wrote:
>  >> Here's another attempt at solving the bug. It adds a flag to
>  >> add_basics_to_problem_data indicating whether or not we are
>  >> interested about
>  >> data from current process. Additionally, it adds optional argument to
>  >> createAlertSignature, which can be used by sealert to pass the
>  >> executable
>  >> directly.
>  >>
>  >
>  > ok, patch seems good. what I would like to see is only addition of an optional
>  > argument to *createAlertSignature* and let setroubleshoot and anaconda fix this
> issue.
>  
>  - Vrata is working on a patch for python-meh and abrt-server to do 
>  exactly this.. 
Am I?
I am working on modifying abrt-server in a way that it will be possible
for python-meh to use the abrt.socket for communication.

>  
>  Vrato, what's the news? 
I believe it's almost done, but since I have some
hurrying Anaconda
stuff now, these things are going slowly. I will send my patches for a
review this week. Once they are approved I'll do the modifications in
python-meh.

>  
>  Thanks,
>  Jirka 
-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic