Bug #2732: char copied to wrong place in SSI output - Lighttpd - lighty labs

Actions

Copy link

Bug #2732

closed

char copied to wrong place in SSI output

Added by fbrosson over 8 years ago. Updated over 8 years ago.

Status:

Fixed

Priority:

Normal

Category:

mod_ssi

Target version:

1.4.40

ASK QUESTIONS IN Forums:

Description

Under special conditions (not yet investigated), mod_ssi produces incorrect output.
Attached is a static file that triggers the bug when served as a SSI document. Joining lines 20 and 21 of that file "un-triggers" the bug.
The version tested is a commit planned for 1.4.40:
https://github.com/lighttpd/lighttpd1.4/commit/a5fcfee6fca39ddc951dda92e6c0665981acd890
BTW, although this is probably not very important to mod_ssi, the document is Valid XHTML according to https://validator.w3.org/.
A few other comments are in the attached file.

Files

mod_ssi_char_copied_to_wrong_place.shtml (8.56 KB) mod_ssi_char_copied_to_wrong_place.shtml

fbrosson, 2016-05-17 14:14

Actions

Copy link

Updated by fbrosson over 8 years ago

Sorry, I forgot to mention a couple of things...
System: Debian testing/stretch (i.e. future Debian 9).
Compiler: gcc (Debian 5.3.1-17) 5.3.1 20160429
Configure options: --with-webdav-props --with-webdav-locks --with-memcached --with-attr --with-gdbm

Actions

Copy link

Updated by gstrauss over 8 years ago

Status changed from New to Patch Pending

Good catch, and excellent test case! When the tag crossed the buffer boundary (8192), the partial tag was not preserved when data was shifted in the buffer. I should have tested that boundary condition. The following adjusts the offset of data in the buffer instead of (incorrectly) resetting the offset.

diff --git a/src/mod_ssi.c b/src/mod_ssi.c
index ffba44a..4dcfdd1 100644
--- a/src/mod_ssi.c
+++ b/src/mod_ssi.c
@@ -1227,16 +1227,16 @@ static void mod_ssi_read_fd(server *srv, connection *con, plugin_data *p, int fd
                                        offset = pretag = 0;
                                        break;
                                } else { /* incomplete directive "<!--#...-->" */
-                                       memmove(buf, buf+prelen, offset-prelen);
-                                       offset = pretag = 0;
+                                       memmove(buf, buf+prelen, (offset -= prelen));
+                                       pretag = 0;
                                        break;
                                }
                        } else if (prelen + 1 == offset || 0 == memcmp(s+1, "!--", offset - prelen - 1)) {
                                if (prelen - pretag && !p->if_is_false) {
                                        chunkqueue_append_mem(con->write_queue, buf+pretag, prelen-pretag);
                                }
-                               memcpy(buf, buf+prelen, offset-prelen);
-                               offset = pretag = 0;
+                               memcpy(buf, buf+prelen, (offset -= prelen));
+                               pretag = 0;
                                break;
                        }
                        /* loop to look for next '<' */

Actions

Copy link

Updated by gstrauss over 8 years ago

Separately, I noticed that there might be an additional adjustment that might need to be made to the new mod_ssi parser. What is your expectation of SSI expressions within HTML comments (" -->")? Should the SSI expression be evaluated? Emitted as-is? Removed from the comment and replaced with a nothing ("")? Current behavior is to execute the SSI expression, as SSI expressions can occur anywhere in the HTML document.

Aside: to your question on a prior github pull request: it is fine to either open a github issue or to open an issue here on the redmine tracker. It is not necessary to do both.

Actions

Copy link

Updated by fbrosson over 8 years ago

I knew you would quickly find out what was going on, and I'm glad I managed to create the test case;-)

Regarding the SSI commands inside HTML comments, I would say that the current behaviour is perfect. It is indeed much simpler the way it is because it does not need to keep track of whether or not we're inside an HTML comment. And I would even say that being able to insert dynamic content inside HTML comments is a very interesting feature, so keeping the current behaviour is just perfect. If someone wants to disable an SSI command then this can be achieved by replacing the sharp sign with an equal sign or a third dash, or anything else.

Regarding the best place for creating tickets, it's cool to be able to create them in any side (and it is indeed smart to create them only at one place.)

Off topic: Haiku has no libsocket nor libnsl. All network functions (e.g. gethostbyname(), accept() and friends) are in libnetwork. This is why I patched configure.ac (on haikuports) to add:

dnl On Haiku the accept function is in libnetwork
AC_SEARCH_LIBS(accept,network)

just after

AC_SEARCH_LIBS(gethostbyname,nsl socket)
AC_SEARCH_LIBS(hstrerror,resolv)

It works fine on Haiku but I don't know, however, if this is the best way to patch configure.ac and I haven't checked that it does not break other systems.
BTW, I noticed that the configure.ac in cURL knows a lot of operating systems.
Should I create a separate ticket with a suggested patch to detect libnetwork? Thanks!

Actions

Copy link