Bug 137926 - storeAsURL regression between libreoffice < 7 and libreoffice >=7
Summary: storeAsURL regression between libreoffice < 7 and libreoffice >=7
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: sdk (show other bugs)
Version:
(earliest affected)
7.0.3.1 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2020-11-02 08:40 UTC by libreoffice
Modified: 2022-10-13 11:47 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
test file (33.03 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-11-02 08:42 UTC, libreoffice
Details
script (2.67 KB, text/x-python)
2020-11-02 08:42 UTC, libreoffice
Details
script I used to test the regression (3.86 KB, text/x-python)
2021-03-15 23:00 UTC, libreoffice
Details
test file (19 bytes, text/plain)
2021-03-15 23:00 UTC, libreoffice
Details
patch example (2.07 KB, patch)
2021-03-15 23:12 UTC, libreoffice
Details

Note You need to log in before you can comment on or make changes to this bug.
Description libreoffice 2020-11-02 08:40:57 UTC
Description:
In libreoffice < 7, the python API "storeAsURL" work.
In libreoffice >= 7, the python API "storeAsURL" hang and crash with the code: 

__main__.ErrorCodeIOException: SfxBaseModel::impl_store <file:///home/user/libreoffice_bug/docx.docx.nopassword> failed: 0x20d(Error Area:Io Class:General Code:13) /build/libreoffice-fresh/src/libreoffice-7.0.2.2/sfx2/source/doc/sfxbasemodel.cxx:3153

I will attach all the files required to reproduce this issue easily.
(I am fairly new with libreoffice, tried to find any announcement for breaking change between <7 and >=7, but found nothing relevant.)

Steps to Reproduce:
1. python3 ./remove_password.py /{ABSOLUTE_PATH}/docx.docx ''


Actual Results:
In libreoffice <7: A file is correctly created
In libreoffice >=7: Hang or crash 

Expected Results:
A file should be created


Reproducible: Always


User Profile Reset: No



Additional Info:
Non working environment: LibreOffice 7.0.2.2 00(Build:2), archlinux
Working environment: LibreOffice 6.4.7.2 40(Build:2), fedora 32
Comment 1 libreoffice 2020-11-02 08:42:02 UTC
Created attachment 166919 [details]
test file
Comment 2 libreoffice 2020-11-02 08:42:19 UTC
Created attachment 166920 [details]
script
Comment 3 libreoffice 2020-11-02 08:44:15 UTC
To make it easier to debug I only attached 1 test file ("docx.docx"), this document is NOT protected by a password. But the issue is also producible with password protected file
Comment 4 libreoffice 2021-03-15 22:57:33 UTC
I searched a bit deeper and found 2 regression between 6.4.X and 7.X.
This ticket will be for only one of those, I will open a new ticket for other. 

Commit 0de0b1b64a1c122254bb821ea0eb9b038875e8d4 
( https://gerrit.libreoffice.org/c/core/+/104345 / https://github.com/LibreOffice/core/commit/0de0b1b64a1c122254bb821ea0eb9b038875e8d4 ) 

From what I understand (never wrote nor read any meaningful C++ nor looked in the libreoffice codebase before).
Before this commit: it checked if a password have been submit, if that is the case, then do something.
After this commit: it check if the document contains encrypted data, if that is the case, then do something.

This change of behavior broke at least 1 use case:
Trying to open a non encrypted file using the API/uno while submitting a password. 

I tried to rewrote this part to fix this use-case:

```
// Also, ( maybe the new itemset contains new values, otherwise they will be empty )
     bool bPasswordProtected = true;                                             
      if (xMergedParams->HasItem(SID_ENCRYPTIONDATA))                             
     {                                                                           
          const SfxUnoAnyItem* pEncryptionDataItem                                
              = xMergedParams->GetItem<SfxUnoAnyItem>(SID_ENCRYPTIONDATA, false); 
          if (pEncryptionDataItem)                                                
          {                                                                       
              uno::Sequence<beans::NamedValue> aEncryptionData;                   
              pEncryptionDataItem->GetValue() >>= aEncryptionData;                
              for (const auto& rItem : std::as_const(aEncryptionData))            
              {                                                                   
                  if (rItem.Name == "CryptoType")                                 
                  {                                                               
                      OUString aValue;                                            
                      rItem.Value >>= aValue;                                     
                      if (aValue != "StrongEncryptionDataSpace")                  
                      {                                                           
                          // This is not just a password protected document. Let's keep encryption data as is.
                          bPasswordProtected = false;                             
                      }                                                           
                      break;                                                      
                  }                                                               
              }                                                                   
          }                                                                       
     }else if (xMergedParams->HasItem( SID_PASSWORD )){                          
         bPasswordProtected = true;                                              
     }                                                                           
     if (bPasswordProtected){                                                    
         // For password protected documents remove encryption data during "Save as..."
         xMergedParams->ClearItem(SID_PASSWORD);                                 
         xMergedParams->ClearItem(SID_ENCRYPTIONDATA);                           
      }               
```

(Basically, adding 1 "else if", and moving a block. Didn't sent a proper pull request yet, probably later this week)
I recompiled and I confirm that with this modification it work for my usecase. 


Between my previous comments and now I updated my test script and input file, I will attach the new versions in this ticket. 
The new command is

```
./remove_password.py /home/user/qubes-app-linux-pdf-converter/tests/files_success/csv.tmp ''
```

("csv.tmp" is a standard csv file)


With the patch, the output is 
```
libreoffice process
wait for socket
:file:///home/user/qubes-app-linux-pdf-converter/tests/files_success/csv.tmp:
```
, the process terminate within seconds and the file is correctly created.

Without the patch, the output is
```
libreoffice process
wait for socket
:file:///home/user/qubes-app-linux-pdf-converter/tests/files_success/csv.tmp:
```
, the process does not terminate (hang indefinitly), and when I send a "CTRL+C" it display the following error "__main__.ErrorCodeIOException: SfxBaseModel::impl_store <file:///home/user/qubes-app-linux-pdf-converter/tests/files_success/csv.tmp.nopassword> failed: 0x20d(Error Area:Io Class:General Code:13) /home/user/core/sfx2/source/doc/sfxbasemodel.cxx:3153
"
Comment 5 libreoffice 2021-03-15 23:00:13 UTC
Created attachment 170499 [details]
script I used to test the regression
Comment 6 libreoffice 2021-03-15 23:00:51 UTC
Created attachment 170500 [details]
test file
Comment 7 libreoffice 2021-03-15 23:12:02 UTC
Created attachment 170502 [details]
patch example

Patch applied on my side, that apparently fix the issue.
I compiled libreoffice and tested it worked as expected with my script. Nothing more.
Comment 8 Roman Kuznetsov 2021-08-15 19:04:18 UTC
Xisco, could you please look at this problem, the script and at the patch too. Thank you
Comment 9 Ross Johnson 2021-10-05 04:49:36 UTC
(In reply to libreoffice from comment #4)
> I tried to rewrote this part to fix this use-case:
> 
> ```
> // Also, ( maybe the new itemset contains new values, otherwise they will be
> empty )
>      bool bPasswordProtected = true;                                        
> 
>       if (xMergedParams->HasItem(SID_ENCRYPTIONDATA))                       
>      {                                                                      
>           const SfxUnoAnyItem* pEncryptionDataItem                          
>               = xMergedParams->GetItem<SfxUnoAnyItem>(SID_ENCRYPTIONDATA,
> false); 
>           if (pEncryptionDataItem)                                          
>           {                                                                 
>               uno::Sequence<beans::NamedValue> aEncryptionData;             
>               pEncryptionDataItem->GetValue() >>= aEncryptionData;          
>               for (const auto& rItem : std::as_const(aEncryptionData))      
>               {                                                             
>                   if (rItem.Name == "CryptoType")                           
>                   {                                                         
>                       OUString aValue;                                      
>                       rItem.Value >>= aValue;                               
>                       if (aValue != "StrongEncryptionDataSpace")            
>                       {                                                     
>                           // This is not just a password protected document.
> Let's keep encryption data as is.
>                           bPasswordProtected = false;                       
>                       }                                                     
>                       break;                                                
>                   }                                                         
>               }                                                             
>           }                                                                 
>      }else if (xMergedParams->HasItem( SID_PASSWORD )){                     
>          bPasswordProtected = true;                                         
>      }                                                                      
>      if (bPasswordProtected){                                               
>          // For password protected documents remove encryption data during
> "Save as..."
>          xMergedParams->ClearItem(SID_PASSWORD);                            
>          xMergedParams->ClearItem(SID_ENCRYPTIONDATA);                      
>       }               
> ```

The first:
     bool bPasswordProtected = true;

in your patch makes your 'else if' redundant, so is surely not what you intended.
Comment 10 Buovjaga 2022-10-13 11:47:32 UTC
It hangs for me as well and I tested with linux-64-7.0 bibisect repo and confirm the finding.