Re: [PATCH] fix use-after-free in mod_dav_svn's log_warning()

[Stefan Fuhrmann]

On 17.12.18 10:44, Stefan Sperling wrote:

I have hit a use-after-free in mod_dav_svn while running SVN's regression
tests on OpenBSD with httpd 2.4. This problem was apparently known to the
author; see the comment which is removed in the diff below.

In short, the request structure used as logging context can already be
freed before log_warning() runs. The repository structure is allocated
in the request's pool, which means the repository is closed when the
request pool gets freed. I am not sure which, if any, ordering guarantess
exist in this situation. But the patch below switches the logging context
to the connection instead of the request and I have been unable to reproduce
the issue since.

Is there a better solution?
Does my proposed solution lose too much logging context?

Error handling during tear-down is always a bit messy.

I guess not using the request struct will mean we cannot
tell what operation cased a problem - ever (not sure,
though). If true, that's a high cost.

One way to improve your solution would be to make
the error handling degrade as the objects are being
destroyed. But it adds / duplicates a bit of code:

* keep the logging based on request context
* apr_pool_pre_cleanup_register on the request pool,
  a switch to the new connection-based logging

Greetings from CN!
-- Stefan^2

Re: [swig-py3][patch] interfacing bytes object instead of str

[futatuki]

On 12/17/18 05:50, Yasuhito FUTATSUKI wrote:

I found exception error messages isn't unified for
('a bytes', 'a bytes object', 'a Bytes object'),
and nothing but these.


I've updated the patch including fix above (unified to
'a bytes objects') and type casting between signed and
unsigned for len argment in svn_stream_write(),
for swig-py3@1849073

Cheers,
--
Yasuhito FUTATSUKI
Index: subversion/bindings/swig/core.i
===
--- subversion/bindings/swig/core.i	(revision 1849073)
+++ subversion/bindings/swig/core.i	(working copy)
@@ -130,6 +130,8 @@
 /* svn_stream_checksummed would require special attention to wrap, because
  * of the read_digest and write_digest parameters. */
 %ignore svn_stream_checksummed;
+// svn_stream_read_full
+// svn_stream_read2
 // svn_stream_read
 // svn_stream_write
 // svn_stream_close
@@ -420,7 +422,7 @@
 
 #ifdef SWIGPYTHON
 %typemap(argout) (char *buffer, apr_size_t *len) {
-  %append_output(PyStr_FromStringAndSize($1, *$2));
+  %append_output(PyBytes_FromStringAndSize($1, *$2));
   free($1);
 }
 #endif
@@ -442,16 +444,18 @@
 */
 #ifdef SWIGPYTHON
 %typemap(in) (const char *data, apr_size_t *len) ($*2_type temp) {
-if (!PyStr_Check($input)) {
+char *tmpdata;
+Py_ssize_t length;
+if (!PyBytes_Check($input)) {
 PyErr_SetString(PyExc_TypeError,
-"expecting a string for the buffer");
+"expecting a bytes object for the buffer");
 SWIG_fail;
 }
-/* Explicitly cast away const from PyStr_AsUTF8AndSize (in Python 3). The
-   swig generated argument ($1) here will be "char *", but since its only
-   use is to pass directly into the "const char *" argument of the wrapped
-   function, this is safe to do. */
-$1 = (char *)PyStr_AsUTF8AndSize($input, );
+if (PyBytes_AsStringAndSize($input, , ) == -1) {
+SWIG_fail;
+}
+temp = ($*2_type)length;
+$1 = tmpdata;
 $2 = ($2_ltype)
 }
 #endif
@@ -489,6 +493,20 @@
 #endif
 
 /* ---
+   fix up the svn_stream_readline() eol argument
+*/
+#ifdef SWIGPYTHON
+%typemap(in) (const char *eol) {
+if (!PyBytes_Check($input)) {
+PyErr_SetString(PyExc_TypeError,
+"expecting a bytes object for the eol");
+SWIG_fail;
+}
+$1 = PyBytes_AsString($input);
+}
+#endif
+
+/* ---
auth parameter set/get
 */
 
Index: subversion/bindings/swig/include/svn_string.swg
===
--- subversion/bindings/swig/include/svn_string.swg	(revision 1849073)
+++ subversion/bindings/swig/include/svn_string.swg	(working copy)
@@ -28,11 +28,14 @@
 
 /* ---
generic OUT param typemap for svn_string(buf)_t. we can share these
-   because we only refer to the ->data and ->len values.
+   except in the case of Python, because we only refer to the ->data and ->len
+   values. In case of Python, svn_stringbuf_t represents raw bytes and
+   svn_string_t represents string in most cases, so it is convenient to
+   distinguish them.
 */
 #ifdef SWIGPYTHON
 
-%typemap(argout) RET_STRING {
+%typemap(argout) svn_string_t ** {
 PyObject *s;
 if (*$1 == NULL) {
 Py_INCREF(Py_None);
@@ -45,6 +48,19 @@
 }
 %append_output(s);
 }
+
+%typemap(argout) svn_stringbuf_t ** {
+PyObject *s;
+if (*$1 == NULL) {
+Py_INCREF(Py_None);
+s = Py_None;
+} else {
+s = PyBytes_FromStringAndSize((*$1)->data, (*$1)->len);
+if (s == NULL)
+SWIG_fail;
+}
+%append_output(s);
+}
 #endif
 #ifdef SWIGPERL
 %typemap(argout) RET_STRING {
@@ -65,10 +81,12 @@
 }
 #endif
 
+#ifndef SWIGPYTHON
 %apply RET_STRING {
   svn_string_t **,
   svn_stringbuf_t **
 };
+#endif
 
 /* ---
TYPE: svn_stringbuf_t
@@ -76,6 +94,22 @@
 
 #ifdef SWIGPYTHON
 %typemap(in) svn_stringbuf_t * {
+if (!PyBytes_Check($input)) {
+PyErr_SetString(PyExc_TypeError, "not a bytes object");
+SWIG_fail;
+}
+{
+  Py_ssize_t strBufLen;
+  char *strBufChar;
+  if (-1 == PyBytes_AsStringAndSize($input, , )) {
+SWIG_fail;
+  }
+  $1 = svn_stringbuf_ncreate(strBufChar, strBufLen,
+ /* ### gah... what pool to use? */
+ _global_pool);
+}
+}
+%typemap(in) svn_stringbuf_t *output {
 if (!PyStr_Check($input)) {
 PyErr_SetString(PyExc_TypeError, "not a string");
 SWIG_fail;
@@ -264,6 +298,18 @@
 }
 %append_output(s);
 }
+%typemap(argout) const char **eol {
+PyObject *s;
+if (*$1 == NULL) {
+Py_INCREF(Py_None);
+s = Py_None;
+} else {
+s 

Re: [swig-py3][patch] interfacing bytes object instead of str

[futatuki]

On 12/17/18 18:35, Daniel Shahaf wrote:

Yasuhito FUTATSUKI wrote on Mon, Dec 17, 2018 at 05:50:03 +0900:

On 12/17/18 2:08 AM, Troy Curtis Jr wrote:

On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf 
wrote:


Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:

But there was one item I wanted to talk about related to the patch.  I
agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is the
right general path,


Would svn_stringbuf_t map better to bytearray?  Sorry, that didn't occur
to me yesterday.


I think if proxy object for svn_stringbuf_t can support buffer protocol,
no need to translate to bytes object for return values, however,
for input values, it is still need to translation to svn_stringbuf_t
with entity copy except proxy objects.

(I already agreed that propety values should be mapped to bytes objects,
so I ommit latter half.)

Cheers,
--
Yasuhito FUTATSUKI

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri

[Julian Foad]

Branko Čibej wrote:
>    (svn_relpath__internal_style): Change prototype so that the function 
>  can
> return an error instead of aborting if anything goes wrong.
> [...]  I'll rename the function at the same time.

See IRC: I suggest a name like "svn_relpath__from_rel_dirent", and its use in 
JavaHL appears to be erroneous or at the very least error-prone.

-- 
- Julian

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri

[Branko Čibej]

On 17.12.2018 13:28, Daniel Shahaf wrote:
> Branko Čibej wrote on Mon, 17 Dec 2018 13:19 +0100:
>> On 17.12.2018 13:11, Daniel Shahaf wrote:
>>> br...@apache.org wrote on Mon, 17 Dec 2018 11:26 +:
 * subversion/include/svn_dirent_uri.h
   (svn_relpath__internal_style): Change prototype so that the function can
return an error instead of aborting if anything goes wrong.
>>> Shall we move the declaration to subversion/include/private/ while we're
>>> at it?  That will ensure that API consumers that called this function
>>> --- yes, they shouldn't have, but they may have anyway --- don't
>>> accidentally call the re-signatured function (after upgrading libsvn.so
>>> without recompiling) and get hard-to-trace garbage.
>>
>> I've been meaning to raise the same question. The only non-library user
>> is svndumpfilter, but we "cheat" in the command-line client, too.
>>
>> If no-one objects, I'll move this declaration to somewhere in
>> subversion/include/private. I'm not sure where though, there's no really
>> appropriate private header there (svn_subr_private.h and
>> svn_string_private.h are the obvious candidates).
> I'd say the obvious candidate is a new svn_dirent_uri_private.h.
>
> But I thinko'd earlier.  Moving the declaration won't affect the ABI
> compatibility issue; for that we'd have to rename the function as well.
>
> I think it's plausible that a third party library user may be calling this
> function since it doesn't specifically have a doxygen note warning that
> it's private --- notwithstanding it being named with double underscores.

True ...  I'll rename the function at the same time.

-- Brane

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri

[Daniel Shahaf]

Branko Čibej wrote on Mon, 17 Dec 2018 13:19 +0100:
> On 17.12.2018 13:11, Daniel Shahaf wrote:
> > br...@apache.org wrote on Mon, 17 Dec 2018 11:26 +:
> >> * subversion/include/svn_dirent_uri.h
> >>   (svn_relpath__internal_style): Change prototype so that the function can
> >>return an error instead of aborting if anything goes wrong.
> > Shall we move the declaration to subversion/include/private/ while we're
> > at it?  That will ensure that API consumers that called this function
> > --- yes, they shouldn't have, but they may have anyway --- don't
> > accidentally call the re-signatured function (after upgrading libsvn.so
> > without recompiling) and get hard-to-trace garbage.
> 
> 
> I've been meaning to raise the same question. The only non-library user
> is svndumpfilter, but we "cheat" in the command-line client, too.
> 
> If no-one objects, I'll move this declaration to somewhere in
> subversion/include/private. I'm not sure where though, there's no really
> appropriate private header there (svn_subr_private.h and
> svn_string_private.h are the obvious candidates).

I'd say the obvious candidate is a new svn_dirent_uri_private.h.

But I thinko'd earlier.  Moving the declaration won't affect the ABI
compatibility issue; for that we'd have to rename the function as well.

I think it's plausible that a third party library user may be calling this
function since it doesn't specifically have a doxygen note warning that
it's private --- notwithstanding it being named with double underscores.

Cheers,

Daniel

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri

[Branko Čibej]

On 17.12.2018 13:11, Daniel Shahaf wrote:
> br...@apache.org wrote on Mon, 17 Dec 2018 11:26 +:
>> * subversion/include/svn_dirent_uri.h
>>   (svn_relpath__internal_style): Change prototype so that the function can
>>return an error instead of aborting if anything goes wrong.
> Shall we move the declaration to subversion/include/private/ while we're
> at it?  That will ensure that API consumers that called this function
> --- yes, they shouldn't have, but they may have anyway --- don't
> accidentally call the re-signatured function (after upgrading libsvn.so
> without recompiling) and get hard-to-trace garbage.


I've been meaning to raise the same question. The only non-library user
is svndumpfilter, but we "cheat" in the command-line client, too.

If no-one objects, I'll move this declaration to somewhere in
subversion/include/private. I'm not sure where though, there's no really
appropriate private header there (svn_subr_private.h and
svn_string_private.h are the obvious candidates).

-- Brane

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri

[Daniel Shahaf]

br...@apache.org wrote on Mon, 17 Dec 2018 11:26 +:
> * subversion/include/svn_dirent_uri.h
>   (svn_relpath__internal_style): Change prototype so that the function can
>return an error instead of aborting if anything goes wrong.

Shall we move the declaration to subversion/include/private/ while we're
at it?  That will ensure that API consumers that called this function
--- yes, they shouldn't have, but they may have anyway --- don't
accidentally call the re-signatured function (after upgrading libsvn.so
without recompiling) and get hard-to-trace garbage.

[PATCH] fix use-after-free in mod_dav_svn's log_warning()

[Stefan Sperling]

I have hit a use-after-free in mod_dav_svn while running SVN's regression
tests on OpenBSD with httpd 2.4. This problem was apparently known to the
author; see the comment which is removed in the diff below.

In short, the request structure used as logging context can already be
freed before log_warning() runs. The repository structure is allocated
in the request's pool, which means the repository is closed when the
request pool gets freed. I am not sure which, if any, ordering guarantess
exist in this situation. But the patch below switches the logging context
to the connection instead of the request and I have been unable to reproduce
the issue since.

Is there a better solution?
Does my proposed solution lose too much logging context?

Below is a crash trace; On OpenBSD, freed memory gets overwritten with 0xdf.

$ egdb /home/stsp/svn/prefix/httpd/bin/httpd httpd.c>  <
GNU gdb (GDB) 7.12.1
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-openbsd6.4".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /home/stsp/svn/prefix/httpd/bin/httpd...done.
[New process 351753]
Core was generated by `httpd'.
Program terminated with signal SIGBUS, Bus error.
#0  0x08baba525290 in ap_get_request_module_loglevel (r=0x8bcbc3b0400,
module_index=-1) at /home/stsp/svn/src/httpd-2.4.37/server/util_debug.c:200
200 return l->level;
(gdb) p l
$1 = (const struct ap_logconf *) 0xdfdfdfdfdfdfdfdf
(gdb) up
#1  0x08baba50f45c in log_error_core (
file=0x8bd831429d6 "subversion/mod_dav_svn/repos.c", line=1246,
module_index=-1, level=3, status=20014, s=0xdfdfdfdfdfdfdfdf,
c=0xdfdfdfdfdfdfdfdf, r=0x8bcbc3b0400, pool=0x0, fmt=0x8bd83149264 "%s%s",
args=0x7f7ceb20) at /home/stsp/svn/src/httpd-2.4.37/server/log.c:1169
1169int configured_level = r ? ap_get_request_module_loglevel(r, 
module_index):
(gdb) up
#2  0x08baba50fe9b in ap_log_rerror_ (
file=0x8bd831429d6 "subversion/mod_dav_svn/repos.c", line=1246,
module_index=-1, level=3, status=20014, r=0x8bcbc3b0400,
fmt=0x8bd83149264 "%s%s")
at /home/stsp/svn/src/httpd-2.4.37/server/log.c:1368
1368log_error_core(file, line, module_index, level, status, r->server, 
NULL, r,
(gdb) p *r
$4 = {pool = 0xdfdfdfdfdfdfdfdf, connection = 0xdfdfdfdfdfdfdfdf,
  server = 0xdfdfdfdfdfdfdfdf, next = 0xdfdfdfdfdfdfdfdf,
  prev = 0xdfdfdfdfdfdfdfdf, main = 0xdfdfdfdfdfdfdfdf,
  the_request = 0xdfdfdfdfdfdfdfdf , assbackwards = -538976289, proxyreq = -538976289,
  header_only = -538976289, proto_num = -538976289,
  protocol = 0xdfdfdfdfdfdfdfdf ,
  hostname = 0xdfdfdfdfdfdfdfdf , request_time = -2314885530818453537,
  status_line = 0xdfdfdfdfdfdfdfdf , status = -538976289, method_number = -538976289,
  method = 0xdfdfdfdfdfdfdfdf , allowed = -2314885530818453537,
  allowed_xmethods = 0xdfdfdfdfdfdfdfdf, allowed_methods = 0xdfdfdfdfdfdfdfdf,
  sent_bodyct = -2314885530818453537, bytes_sent = -2314885530818453537,
  mtime = -2314885530818453537,
  range = 0xdfdfdfdfdfdfdfdf , clength = -2314885530818453537, chunked = -538976289,
  read_body = -538976289, read_chunked = -538976289,
  expecting_100 = 3755991007, kept_body = 0xdfdfdfdfdfdfdfdf,
  body_table = 0xdfdfdfdfdfdfdfdf, remaining = -2314885530818453537,
  read_length = -2314885530818453537, headers_in = 0xdfdfdfdfdfdfdfdf,
  headers_out = 0xdfdfdfdfdfdfdfdf, err_headers_out = 0xdfdfdfdfdfdfdfdf,
  subprocess_env = 0xdfdfdfdfdfdfdfdf, notes = 0xdfdfdfdfdfdfdfdf,
  content_type = 0xdfdfdfdfdfdfdfdf ,
  handler = 0xdfdfdfdfdfdfdfdf ,
  content_encoding = 0xdfdfdfdfdfdfdfdf , content_languages = 0xdfdfdfdfdfdfdfdf,
  vlist_validator = 0xdfdfdfdfdfdfdfdf ,
  user = 0xdfdfdfdfdfdfdfdf ,
  ap_auth_type = 0xdfdfdfdfdfdfdfdf ,
  unparsed_uri = 0xdfdfdfdfdfdfdfdf ,
  uri = 0xdfdfdfdfdfdfdfdf ,
  filename = 0xdfdfdfdfdfdfdfdf ,
  canonical_filename = 0xdfdfdfdfdfdfdfdf ,
  path_info = 0xdfdfdfdfdfdfdfdf ,
  args = 0xdfdfdfdfdfdfdfdf , used_path_info = -538976289, eos_sent = -538976289,
  per_dir_config = 0xdfdfdfdfdfdfdfdf, request_config = 0xdfdfdfdfdfdfdfdf,
  log = 0xdfdfdfdfdfdfdfdf,
  log_id = 0xdfdfdfdfdfdfdfdf , htaccess = 0xdfdfdfdfdfdfdfdf, output_filters = 
0xdfdfdfdfdfdfdfdf,
  input_filters = 0xdfdfdfdfdfdfdfdf,
  proto_output_filters = 0xdfdfdfdfdfdfdfdf,
  proto_input_filters = 0xdfdfdfdfdfdfdfdf, no_cache = -538976289,
  no_local_copy = 

Re: [swig-py3][patch] interfacing bytes object instead of str

[Daniel Shahaf]

Yasuhito FUTATSUKI wrote on Mon, Dec 17, 2018 at 05:50:03 +0900:
> On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
> > On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf 
> > wrote:
> > 
> > > Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
> > > > But there was one item I wanted to talk about related to the patch.  I
> > > > agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is 
> > > > the
> > > > right general path,

Would svn_stringbuf_t map better to bytearray?  Sorry, that didn't occur
to me yesterday.

> > > Are you sure about this?  Property values can be binary data (so not
> > > representable as 'str') but are usually represented by «svn_string_t *»;
> > > for example, svn_repos_parse_fns3_t::set_node_property().
> > > 
> > 
> > Ah yes, I suppose this also has bearing on the discussion in the other
> > thread relating
> > to returning property values.  With that in mind, perhaps your suggestion
> > of effectively
> > defaulting to always being Bytes for char * , svn_string_t, and
> > svn_stringbuf_t unless there
> > is a specific circumstance not to makes the most sense as the general
> > principle.
> 
> To treat all those values as bytes in wrapper makes the wrapper is lower
> level, keeping raw access like device drivers. On the other hand,
> to make contexts, to make semantic differences in wrapper makes it more
> higher level. So I think those are not the general principle but just a
> policy of the wrapper design.

In general, you're right.  The swig bindings are not Pythonic; to use
them requires using C idioms while writing Python.  There's certainly
room for higher-level abstractions — I think the javahl and
python-ctypes bindings are examples — but the trade-off is that they
can't be autogenerated like the swig bindings can.

However, all that aside, I think this general discussion doesn't apply
here.  The value of the svn_string_t* parameter of 
svn_repos_parse_fns3_t::set_node_property()
may be *unrepresentable* as a py3 str object.  It isn't a question of
C-like idiom versus a Pythonic idiom; the bytes class can can represent
all possible values, the str class cannot.

Consider «svn propset -F /bin/sh foo iota».  How would you represent
that value as str?

> I, as a consumer of the Python bindings, prefer the latter if it is
> clear that what are bytes and what are str, however I also agree with
> former if the distinction is too complex for both of consumers and
> developers.

Cheers,

Daniel