Skip to content

Commit

Permalink
Don't use manual memory management in wxrc
Browse files Browse the repository at this point in the history 
Use std::unique_ptr<> instead of manually deleting the object and
std::vector<> instead of manually allocating and deleting the buffers.

No real changes.
  • Loading branch information
@vadz
vadz committed May 22, 2024
1 parent a01c470 commit 3c41a68
Showing 1 changed file with 8 additions and 12 deletions.
20 changes: 8 additions & 12 deletions utils/wxrc/wxrc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include "wx/mimetype.h"
#include "wx/vector.h"

#include <memory>

class XRCWidgetData
{
public:
Expand Down Expand Up @@ -620,8 +622,8 @@ static wxString FileToCppArray(wxString filename, int num)
// we cannot use string literals because MSVC is dumb wannabe compiler
// with arbitrary limitation to 2048 strings :(

unsigned char *buffer = new unsigned char[lng];
file.Read(buffer, lng);
std::vector<unsigned char> buffer(lng);
file.Read(&buffer[0], lng);

This comment has been minimized.

Sign in to view

Copy link
@DoctorNoobingstoneIPresume

DoctorNoobingstoneIPresume May 27, 2024

Thank you for the update !! :)

I think that f lng is zero, &buffer [0] may trigger (at runtime) failed sanity checks with some debug versions of some implementations of STL.

I remember this when #define-ing _STLP_DEBUG with STLport.

But I think that such checks are valid, because (as per https://timsong-cpp.github.io/cppwp/n4861/sequence.reqmts#14 -- the row with a[n] near the bottom of the table) evaluating &v [0] theoretically implies dereferencing v.begin (), which in the case of an empty vector is a non-dereferenceable iterator.

May I suggest replacing &buffer [0] with buffer.data () (which is the same as &buffer [0] except that it is also safe in the case of empty vectors), please ?

This comment has been minimized.

Sign in to view

Copy link
@vadz

vadz May 27, 2024

Author Contributor

Hmm, thanks, I didn't realize that lng could be zero here, but it seems to be indeed possible. I believe that it would be best to handle this as an error because embedding empty file doesn't seem to make sense, but please let me know if I'm missing something here?

This comment has been minimized.

Sign in to view

Copy link
@DoctorNoobingstoneIPresume

DoctorNoobingstoneIPresume May 29, 2024

Thank you !! :) Personally, I would allow zero-length resources if alright with you (as it might simplify customer-specific tooling, for resources generated by scripts/other external sources), but I would check that file.Read (buffer.data (), lng) has indeed been able to read lng bytes from the file.

Even if we disallow zero-length resources (e.g. by a check on the lines just above), we might still want to replace &buffer [0] with buffer.data () or maybe add a comment specifying that there has been a check above for buffer not to be empty -- because it is a little bit subtle (and it does not usually trigger run-time errors except with some debug versions of STL.

For example:

#define _GLIBCXX_DEBUG 1
#include <vector>
#include <iostream>

int main ()
{
    std::vector <float> v;
    std::cout << &v [0] << '\n';
}

produces (with g++ 11):

/usr/include/c++/11/debug/vector:438:
In function:
    std::debug::vector<_Tp, _Allocator>::reference std::debug::vector<_Tp,
    _Allocator>::operator[](std::debug::vector<_Tp, _Allocator>::size_type)
    [with _Tp = float; _Allocator = std::allocator<float>; std::
    debug::vector<_Tp, _Allocator>::reference = float&; std::
    debug::vector<_Tp, _Allocator>::size_type = long unsigned int]

Error: attempt to subscript container with out-of-bounds index 0, but
container only holds 0 elements.

Objects involved in the operation:
    sequence "this" @ 0x7ffce03b25d0 {
      type = std::debug::vector<float, std::allocator<float> >;
    }
Aborted

for (size_t i = 0, linelng = 0; i < lng; i++)
{
Expand All @@ -636,8 +638,6 @@ static wxString FileToCppArray(wxString filename, int num)
linelng += tmp.length()+1;
}

delete[] buffer;

output += wxT("};\n\n");

return output;
Expand Down Expand Up @@ -711,12 +711,10 @@ void XmlResApp::MakePackageCPP(const wxArrayString& flist)
#if wxUSE_MIMETYPE
else
{
wxFileType *ft = wxTheMimeTypesManager->GetFileTypeFromExtension(ext);
std::unique_ptr<wxFileType>
ft{wxTheMimeTypesManager->GetFileTypeFromExtension(ext)};
if ( ft )
{
ft->GetMimeType(&mime);
delete ft;
}
}
#endif // wxUSE_MIMETYPE

Expand Down Expand Up @@ -776,8 +774,8 @@ static wxString FileToPythonArray(wxString filename, int num)
snum.Printf(wxT("%i"), num);
output = " xml_res_file_" + snum + " = '''\\\n";

unsigned char *buffer = new unsigned char[lng];
file.Read(buffer, lng);
std::vector<unsigned char> buffer(lng);
file.Read(&buffer[0], lng);

for (size_t i = 0, linelng = 0; i < lng; i++)
{
Expand All @@ -802,8 +800,6 @@ static wxString FileToPythonArray(wxString filename, int num)
linelng += tmp.length();
}

delete[] buffer;

output += wxT("'''\n\n");

return output;
Expand Down

0 comments on commit 3c41a68

Please sign in to comment.