Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Other comments:

Other comments:

#This code is so full of bugs. It has no write to work.

This code is so full of bugs. It has no write to work.

###Other comments:

#This code is so full of bugs. It has no write to work.

Other comments:

This code is so full of bugs. It has no write to work.

added 2098 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Since you are using C++ compiler. You should use std::vector<viByte> or std::basic_string<viByte>.
This

This kind of bug is easy to introduce and unless you have good unit test hard to find. So why not use a class that already is unit test and has had 10 years of millions of people lookign at it to make sure it works correctly.

###Other comments:

Broken and illegal.

 buf->str = ( viByte* ) 0x1;

Wrong. buf->str is a pointer and it was expecting a unsigned integer (they are not the same). Use %p to print a pointer value.

 printf(" Before viByteBuf_Prepend => %uc ", buf->str);

Broken and illegal (0x2 is not yours)

 viByteBuf_Prepend( buf, 3, ( viByte* ) 0x2 );

No check for NULL. (calloc can return NULL when it fails).

 viByteBuf* buf = ( viByteBuf* ) calloc( sizeof( viByteBuf ), 1 );

Again no check for NULL.

 buf->str = ( viByte* ) calloc( sizeof( viByte ), buflen );

Is this really what you want? Buflen includes the portion with the '0円' character. You need to be consistent and decide if len does or does not include the null.

 buf->len = buflen;

As pointed out above (write past the end of the buffer).

 buf->str[ buflen ] = '0円';

How do you handle failure?
As pointed out above calloc can fail. How do you propagate that failure to the caller.

 viByteBuf* tmp = viByteBuf_New( totallen );

Why are you nulling the whole array. You are just about to write over it.
Seems like a complete waste of time.

 memset( tmp->str, 0, tmp->len );

Looks like this loop is trying to reverse the string in place.
Which will fail as by the time you overwrite the first half the second half is a copy of the back half. Luckily it completely fails because you don't actually use the index variable.

 for( i = 0; i < buf->len; ++i )
 {
 index = ( buf->len - i ) - 1;
 *strpos = buf->str[ 0 ];
 ++strpos;
 }

Now that you have failed to reverse it. You then overwrite it again with 0 (presumably to cover up the failure to reverse it).

 memset( buf->str, 0, buf->len );

Illegal parameter to printf() again.

 printf( "%uc\n", buf->str );

More copying for unknow reason.

 memcpy( buf->str, tmp->str, tmp->len );

#This code is so full of bugs. It has no write to work.

Since you are using C++ compiler. You should use std::vector<viByte>.
This kind of bug is easy to introduce and unless you have good unit test hard to find. So why not use a class that already is unit test and has had 10 years of millions of people lookign at it to make sure it works correctly.

Since you are using C++ compiler. You should use std::vector<viByte> or std::basic_string<viByte>.

This kind of bug is easy to introduce and unless you have good unit test hard to find. So why not use a class that already is unit test and has had 10 years of millions of people lookign at it to make sure it works correctly.

###Other comments:

Broken and illegal.

 buf->str = ( viByte* ) 0x1;

Wrong. buf->str is a pointer and it was expecting a unsigned integer (they are not the same). Use %p to print a pointer value.

 printf(" Before viByteBuf_Prepend => %uc ", buf->str);

Broken and illegal (0x2 is not yours)

 viByteBuf_Prepend( buf, 3, ( viByte* ) 0x2 );

No check for NULL. (calloc can return NULL when it fails).

 viByteBuf* buf = ( viByteBuf* ) calloc( sizeof( viByteBuf ), 1 );

Again no check for NULL.

 buf->str = ( viByte* ) calloc( sizeof( viByte ), buflen );

Is this really what you want? Buflen includes the portion with the '0円' character. You need to be consistent and decide if len does or does not include the null.

 buf->len = buflen;

As pointed out above (write past the end of the buffer).

 buf->str[ buflen ] = '0円';

How do you handle failure?
As pointed out above calloc can fail. How do you propagate that failure to the caller.

 viByteBuf* tmp = viByteBuf_New( totallen );

Why are you nulling the whole array. You are just about to write over it.
Seems like a complete waste of time.

 memset( tmp->str, 0, tmp->len );

Looks like this loop is trying to reverse the string in place.
Which will fail as by the time you overwrite the first half the second half is a copy of the back half. Luckily it completely fails because you don't actually use the index variable.

 for( i = 0; i < buf->len; ++i )
 {
 index = ( buf->len - i ) - 1;
 *strpos = buf->str[ 0 ];
 ++strpos;
 }

Now that you have failed to reverse it. You then overwrite it again with 0 (presumably to cover up the failure to reverse it).

 memset( buf->str, 0, buf->len );

Illegal parameter to printf() again.

 printf( "%uc\n", buf->str );

More copying for unknow reason.

 memcpy( buf->str, tmp->str, tmp->len );

#This code is so full of bugs. It has no write to work.

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Your problem is here:

buf->str[ buflen ] = '0円';

You allocate a buffer (str) of buflen bytes.
This means you can index it (str) from 0 -> (buflen-1).

Thus the accesses above is writing one past the end of the buffer and corrupting memory.
You probably meant:

buf->str[ len ] = '0円';

Since you are using C++ compiler. You should use std::vector<viByte>.
This kind of bug is easy to introduce and unless you have good unit test hard to find. So why not use a class that already is unit test and has had 10 years of millions of people lookign at it to make sure it works correctly.

lang-c

AltStyle によって変換されたページ (->オリジナル) /