Project

General

Profile

Bug #1483

magnet offset > length bug

Added by Anonymous over 9 years ago. Updated about 1 year ago.

Status:
Fixed
Priority:
Normal
Assignee:
-
Category:
mod_magnet
Target version:
Start date:
Due date:
% Done:

0%

Missing in 1.5.x:

Description

This fails:


lighty.content = {
        { filename = "/tmp/test", offset = 12, length = 1}
}
return 200

because of this code:


                                                if (len < off) {
                                                        return luaL_error(L, "offset > length for '%s'", fn->ptr);
                                                }

in {{{mod@mod_magnet.c@

The code should read something like


  if (sce->st.st_size < len + off) {
     return luaL_error(L, "offset + length > filesize for '%s'", fn->ptr);
  }

-- johannes

mod_magnet_length.patch View (1.01 KB) stbuehler, 2008-03-01 19:33

Associated revisions

Revision 659ce5e7 (diff)
Added by gstrauss about 1 year ago

[mod_magnet] rename var for clarity (fixes #1483)

"length" argument is more accurately described as 0-index end of range

x-ref:
"magnet offset > length bug"
https://redmine.lighttpd.net/issues/1483

History

#1 Updated by Anonymous over 9 years ago

Although using "len + off" is not nice if you want to serve zeroes from /dev/zero which has length zero.

The use case where this came up is my jigdo serving script:
http://git.sipsolutions.net/jigdo2magnet.git

-- johannes

#2 Updated by Anonymous over 9 years ago

Another related bug seems to be in this code:


chunkqueue_append_file(con->write_queue, fn, off, len - off);

as far as I can tell, that should just be


chunkqueue_append_file(con->write_queue, fn, off, len);

because the function is defined like this:


int chunkqueue_append_file(chunkqueue *c, buffer *fn, off_t offset, off_t len);

Also, does it miss error-checking of the return value?

-- johannes

#3 Updated by stbuehler about 9 years ago

The implementation didn't match the documentation - weigon told me to fix the doc and so i did.


<weigon> stbuehler: fix docs for now

The implementation treats length as a second offset and sends the range length-1, i.e. (length-offset) bytes beginning at offset.

I think that should not be the end of this bug (the meaning of length is pretty obvious to me), so i leave it open.

#4 Updated by Anonymous about 9 years ago

I had a patch, would you take it instead? IMHO pretty dumb to have a variable called "length" and treat it as "end".

-- johannes

#5 Updated by stbuehler about 9 years ago

I think it is not a patch that is missing (have one myself), but breaking the current behaviour.

I will append my patch.

#6 Updated by Anonymous about 9 years ago

But nobody could have used the current feature though without reading the code so how can anything break? Oh well. Maybe add a new variable "len" to the dict that has sane semantics, document that and keep "length" around for those adventurous people who read the code? Would be as easy as renaming the "length" variable to "end" in the code, adding a comment that the "length" input really is "end" and calculating "end" based on start/len for the proper case?

-- johannes

#7 Updated by gstrauss about 1 year ago

  • Description updated (diff)
  • Status changed from New to Patch Pending
  • Target version set to 1.4.40

Here's a partial patch to rename the variables in the code and add comments.

Please feel free to enhance this with a patch to look for field named "len" (with length semantics) and prefer it over field named "length" (which the interface misnamed and really means 'end of range').

diff --git a/src/mod_magnet.c b/src/mod_magnet.c
index 2b35079..9afba89 100644
--- a/src/mod_magnet.c
+++ b/src/mod_magnet.c
@@ -733,8 +733,8 @@ static int magnet_attach_content(server *srv, connection *con, lua_State *L, int
                                chunkqueue_append_mem(con->write_queue, data.ptr, data.len);
                        } else if (lua_istable(L, -1)) {
                                lua_getfield(L, -1, "filename");
-                               lua_getfield(L, -2, "length");
-                               lua_getfield(L, -3, "offset");
+                               lua_getfield(L, -2, "length"); /* (0-based) end of range (not actually "length") */
+                               lua_getfield(L, -3, "offset"); /* (0-based) start of range */

                                if (lua_isstring(L, -3)) { /* filename has to be a string */
                                        buffer *fn;
@@ -747,19 +747,19 @@ static int magnet_attach_content(server *srv, connection *con, lua_State *L, int

                                        if (HANDLER_GO_ON == res) {
                                                off_t off = (off_t) luaL_optinteger(L, -1, 0);
-                                               off_t len = (off_t) luaL_optinteger(L, -2, (lua_Integer) sce->st.st_size);
+                                               off_t end = (off_t) luaL_optinteger(L, -2, (lua_Integer) sce->st.st_size);

                                                if (off < 0) {
                                                        buffer_free(fn);
                                                        return luaL_error(L, "offset for '%s' is negative", lua_tostring(L, -3));
                                                }

-                                               if (len < off) {
+                                               if (end < off) {
                                                        buffer_free(fn);
                                                        return luaL_error(L, "offset > length for '%s'", lua_tostring(L, -3));
                                                }

-                                               chunkqueue_append_file(con->write_queue, fn, off, len - off);
+                                               chunkqueue_append_file(con->write_queue, fn, off, end - off);
                                        }

                                        buffer_free(fn);

#8 Updated by gstrauss about 1 year ago

  • Status changed from Patch Pending to Fixed
  • Assignee deleted (jan)

committed in 659ce5e7

Please feel free to enhance this with a subsequent patch to look for field named "len" (with length semantics) and prefer it over field named "length" (which the interface misnamed and really means 'end of range').

Also available in: Atom