Ticket #1483 (new defect)

Opened 7 months ago

Last modified 4 months ago

magnet offset > length bug

Reported by: johannes@sipsolutions.net Assigned to: jan
Priority: normal Milestone:
Component: mod_magnet Version: 1.4.18
Severity: normal Keywords:
Cc: Blocking:
Need Feedback: 0

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_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);
  }

Attachments

mod_magnet_length.patch (1.0 kB) - added by stbuehler on 03/01/2008 07:33:08 PM.

Change History

12/10/2007 03:00:49 PM changed by johannes@sipsolutions.net

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

12/10/2007 03:33:27 PM changed by johannes@sipsolutions.net

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?

03/01/2008 06:06:07 PM changed by stbuehler

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 [offset, 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.

03/01/2008 06:10:22 PM changed by johannes@sipsolutions.net

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

03/01/2008 07:32:50 PM changed by stbuehler

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

I will append my patch.

03/01/2008 07:33:08 PM changed by stbuehler

  • attachment mod_magnet_length.patch added.

03/01/2008 09:10:03 PM changed by johannes@sipsolutions.net

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?

Add/Change #1483 (magnet offset > length bug)




Change Properties