Bug 79874

hmmmm, i'm unconvinced by the patch, can poppler compiled that way still open files with non-ascii characters?

(In reply to comment #1)
> hmmmm, i'm unconvinced by the patch, can poppler compiled that way still
> open files with non-ascii characters?

I'm not sure. It's completely OK to discard the proposed patch.

Another way (possibly better) would be to replace the non-standard _Copy_s with something standard, like copy. The additional bounds checking provided by _Copy_s doesn't seem to bring any benefit since we already know that fileName is large enough (since we just constructed it).

Or then not copy at all - I don't quite get why the copying is needed in the first place. Would something like this do the job (note: untested code):

doc = new PDFDoc((const wchar_t *)filePath.utf16(), filePath.length(), ownerPassword, userPassword);

You are the one with a windows machine, not me, so you tell me :)

(In reply to comment #3)
> You are the one with a windows machine, not me, so you tell me :)

Actually I'm not, I'm compiling for Windows using MinGW on Linux :)

But I do have access to a Windows machine, so I'll try it.

All right, got access to that Windows machine. I tried the last idea I presented, not making any copy at all. I renamed a .pdf file to contain some non-ascii characters and tried if it can still be rendered. Seemed to work fine.

I'll attach a new proposed patch that shows the version of the code I used.

Created attachment 101282 [details] [review]
Proposed patch for the problem, no copying anymore

New proposed fix. No copy of the string is made any more.

Comment on attachment 101282 [details] [review]
Proposed patch for the problem, no copying anymore

Review of attachment 101282 [details] [review]:
-----------------------------------------------------------------

::: qt5/src/poppler-private.h
@@ +86,4 @@
>  		m_filePath = filePath;	
>  
>  #ifdef _WIN32
> +		doc = new PDFDoc((wchar_t *)filePath.utf16(), filePath.length(), ownerPassword, userPassword);

I think that filePath.length() might not match the number of wchar's in filePath.utf16().

For reference, an alternative patch is in bug 80251

(In reply to comment #7)

> >  #ifdef _WIN32
> > +		doc = new PDFDoc((wchar_t *)filePath.utf16(), filePath.length(), ownerPassword, userPassword);
> 
> I think that filePath.length() might not match the number of wchar's in
> filePath.utf16().

Hmm, on second thought, I was wrong in this remark; using QString.length() will work fine to get the number of wchar_t returned by QString.utf16().

Thus the proposed patch looks good to me.

Any info if this will get fixed any time soon? There's some patches already proposed that AFAIK could just be applied as is. If I'm mistaken then please tell me what I could do next.

People is busy and on holidays.

That's good. Have good holidays :)

*** Bug 80251 has been marked as a duplicate of this bug. ***

Pushed, thanks.