Bug #2732


char copied to wrong place in SSI output

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

Target version:


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:
BTW, although this is probably not very important to mod_ssi, the document is Valid XHTML according to
A few other comments are in the attached file.


Actions #1

Updated by fbrosson over 7 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 #2

Updated by gstrauss over 7 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;
                                } else { /* incomplete directive "<!--#...-->" */
-                                       memmove(buf, buf+prelen, offset-prelen);
-                                       offset = pretag = 0;
+                                       memmove(buf, buf+prelen, (offset -= prelen));
+                                       pretag = 0;
                        } 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;
                        /* loop to look for next '<' */
Actions #3

Updated by gstrauss over 7 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 #4

Updated by fbrosson over 7 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 (on haikuports) to add:

dnl On Haiku the accept function is in libnetwork

just after

AC_SEARCH_LIBS(gethostbyname,nsl socket)

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

Actions #5

Updated by gstrauss over 7 years ago

Yes, let's treat Haiku as a separate feature request. Thanks.

Actions #6

Updated by gstrauss over 7 years ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100

Also available in: Atom