Skip to content

Commit

Permalink
Fixed Slice#hexdump index out of bounds
Browse files Browse the repository at this point in the history
Ary Borenszweig committed Aug 12, 2016
1 parent 5118882 commit f1b0505
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/slice.cr
View file
Original file line number Diff line number Diff line change
@@ -323,11 +323,15 @@ struct Slice(T)
lines = full_lines + 1
end

String.new(str_size) do |buffer|
String.new(str_size) do |buf|
index_offset = 0
hex_offset = 10
ascii_offset = 60

# Ensure we don't write outside the buffer:
# slower, but safer (speed is not very important when hexdump is used)
buffer = Slice.new(buf, str_size)

each_with_index do |v, i|
if i % 16 == 0
0.upto(7) do |j|
@@ -351,7 +355,7 @@ struct Slice(T)
hex_offset += 1
end

if i % 16 == 15
if i % 16 == 15 && ascii_offset < str_size
buffer[ascii_offset] = '\n'.ord.to_u8
hex_offset += 27
ascii_offset += 61

6 comments on commit f1b0505

@jhass
Copy link
Member

@jhass jhass commented on f1b0505 Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No spec though? :(

@asterite
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a random segfault for out of bounds access in a pointer. Adding the line that protects the buffer with a slice makes the specs raise IndexError, and the check in the if makes it pass again.

@asterite
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to test that the pointer is not accessed outside of the memory it points to, because I don't know where is that memory...

@jhass
Copy link
Member

@jhass jhass commented on f1b0505 Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, test that an index error is raised now?

@asterite
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be raised unless there's a bug in hexdump

@asterite
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is: there was a bug in hexdump that accessed allocated memory outside of the bounds. There's no way to test that, we can't know what the memory beyond that point is. I discovered this because of this. I added the Slice wrapper to the pointer and got an index out of bounds. I fixed the error and now the specs pass, and there's no bug. I don't know how to really test that it works without doing unsafe access to random memory...

Please sign in to comment.