Bug #1483
closedmagnet offset > length bug
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
Files
Updated by Anonymous almost 17 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
Updated by Anonymous almost 17 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
Updated by stbuehler over 16 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.
Updated by Anonymous over 16 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
Updated by stbuehler over 16 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.
Updated by Anonymous over 16 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
Updated by gstrauss over 8 years 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);
Updated by gstrauss over 8 years 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