MemoryBlock::operator[] Y U NO check offset < size?

General JUCE discussion
1 
/** Returns a byte from the memory block.
    This returns a reference, so you can also use it to set a byte.
*/
template <typename Type>
char& operator[] (const Type offset) const noexcept             { return data [offset]; }

Is there any particular reason this doesn’t have a

if(offset < size) { return data[offset]; }

or even

jassert(offset < size);  return data[offset];

the size member is initialized when you create the memoryBlock. Why not add some safety to accessing specific offsets via the [] operator?

2

It returns a reference so would be impossible for it to deal with an out of bounds index safely. But yes, I guess it could have an assertion in there!

This is a pretty low-level object though, you probably shouldn’t do too much with the bytes directly. If you’re putting data in or out of one you’d probably be better off using streams.

3

interesting.

I’m using it alongside a Blowfish::encrypt()/decrypt().

MemoryBlock mb;
...
bf.encrypt((uint32&)mb[i*4], (uint32&)mb[(i-1)*4]);
4

OK, but

a) The casts you’ve got there are basically nonsense, does that even compile??
b) Blowfish already has methods to operate directly on a MemoryBlock!

5

juce::BlowFish does not have methods to operate on MemoryBlocks. maybe you’re thinking of RSA?

edit: Gasp!!! you added them. i’m still using 4.2.4, not 4.3.0.

those casts compile and run perfectly fine, as long as MemoryBlock is filled up like this:

int lenUTF8 = str.getNumBytesAsUTF8();
int dataLengthRounded = (lenUTF8 + 3) & ~3; //rounds the length UP to a multiple of 4.

//create MemoryBlock that is the correct size for our string, rounded up to the next multiple of 4, and fill it with the string converted to UTF8(equivalent to const char* )
MemoryBlock mb( dataLengthRounded, true);
mb.copyFrom(str.toRawUTF8(), 0, lenUTF8);

this MemoryBlock is what gets passed to encrypt(uint32&, uint32&)

6

It was added late last year:

7

C-style casting a char& to a uint32& really is total gibberish, and very much undefined behaviour… Do you really not even get a compiler warning from that??

And you’re reading beyond the end of the array. Maybe have a look at how it’s done in our implementation, and learn up a bit about C++ casts if you want to do this kind of thing safely!

8

it encrypts and decrypts perfectly fine over here with any sized string being passed in tho, so… :confused:

9

TBH I’ve no idea what I’d even expect the compiler to do when presented with a cast like that. No compiler warnings, really??

Unless you can point at some obscure part of the C++ standard which says it’s a legit thing to do, then you should take my advice and treat it as a very very bad piece of code.

10 
int lenUTF8 = str.getNumBytesAsUTF8();
int dataLengthRounded = (lenUTF8 + 3) & ~3; //rounds the length UP to a multiple of 4.

//create MemoryBlock that is the correct size for our string, rounded up to the next multiple of 4, and fill it with the string converted to UTF8(equivalent to const char* )
MemoryBlock mb( dataLengthRounded, true);
mb.copyFrom(str.toRawUTF8(), 0, lenUTF8);

I don’t understand what the problem is, if the MemoryBlock is being initialized like this. it’s aligned and padded to the size of N uint32’s. MemoryBlock is just a bunch of const char*'s anyway…?

11

This is the code that I’m saying is wrong.

And it needs 8 byte chunks, not 4, so yes, the string padding is probably also wrong. That’s why we provide a method to do this - you should use it!

12

This works perfectly fine:

//1. encrypt [0] thru [N].  [1] thru [N-1] are double encrypted
for( int i = 1; i < mb.getSize() / 4; ++i ) {
    //encrypt [i-1], [i]:  [0,1] [1,2] [2,3] [3,4]... [n-2,n-1] [n-1, n]
    bf.encrypt((uint32&)mb[(i-1) * 4], (uint32&)mb[i * 4]); 
}
int n = ((mb.getSize() / 4) - 1) * 4; 
//double encrypt [0] and [N], since the for() loop missed 'em
bf.encrypt((uint32&)mb[n], (uint32&)mb[0]);
13

Well, if you want to ignore my advice, good luck!

I imagine you’ll regret it at some point in the future, probably when you change a compiler optimisation setting and things start mysteriously failing. By then you’ll have forgotten about this conversation, so will have a hard time finding the problem.

14

This post shows, what it expands to. I don’t consider myself in the position if it is good or bad practise, but I was curious, what it would mean:

I think the notable point is:

If n1 is not an lvalue the code is ill-formed

1 Like
15

The huge difference between these cases is that the source type and destination type is different, i.e. you’re suddenly dealing with aliasing violations, possible alignment errors (depends on source types) not to mention possible trap representations coming from reinterpreting memory, all of these are creating undefined behaviour.

16

ah, now I see what you guys were saying was wrong. using the & sign in the cast. that’s what I get for following the examples in this thread: Encrypt/Decrypt a string

So, in my head, I’m imagining a memoryBlock of size=4 to basically look like this in memory:

[0xAE][0x12][0x45][0x34] or [1010 1110][0001 0010][0100 0101][0011 0100]

each element is a char, so if I cast it to uint32, then (depending on endianness), I’ll get:

0xAE124534 or 0x344512AE
[1010 1110 0001 0010 0100 0101 0011 0100]

I personally don’t care about the value directly. I just care that 0xAE looks like [1010 1110] in memory, and those bytes/bits are what is being transformed inside the BlowFish::encrypt() call. so as long as I pass blowfish 4 valid bytes at a time, I don’t think how the cast happens is something worth worrying over. But you guys have way more experience than me.

All i know is that so far, all of my encrypt()/decrypt calls have resulted in whatever I pass in being decrypted correctly, so that’s why I didn’t stress over the syntax correctness for reading a uint32 from a block of chars. It worked as expected against everything I threw at it…