Project

General

Profile

Bug #2732

char copied to wrong place in SSI output

Added by fbrosson 10 months ago. Updated 10 months ago.

Status:
Fixed
Priority:
Normal
Assignee:
-
Category:
mod_ssi
Target version:
Start date:
2016-05-17
Due date:
% Done:

100%

Missing in 1.5.x:

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.

mod_ssi_char_copied_to_wrong_place.shtml View (8.56 KB) fbrosson, 2016-05-17 14:14

Associated revisions

Revision fde843f6 (diff)
Added by gstrauss 10 months ago

[mod_ssi] fix parse of tag across buf boundary (fixes #2732)

thx fbrosson

x-ref:
"char copied to wrong place in SSI output"
https://redmine.lighttpd.net/issues/2732

History

#1 Updated by fbrosson 10 months 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

#2 Updated by gstrauss 10 months 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 '<' */

#3 Updated by gstrauss 10 months 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 ("<!-- <!--#exec ... --> -->")? 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.

#4 Updated by fbrosson 10 months 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!

#5 Updated by gstrauss 10 months ago

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

#6 Updated by gstrauss 10 months ago

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

Also available in: Atom