MinGW: use namless unions in ext_ad_group_acl #2004

kinkie · 2025-02-27T04:53:47Z

ext_ad_group_acl was written working
around missing support for nameless unions
which are now required to build it.

error: 'IADs' {aka 'struct IADs'} has no member named 'lpVtbl'
error: 'VARIANT' {aka 'struct tagVARIANT'} has no member named 'n1'

rousskov

I do not know enough to review this at low level, but please make the following PR title/description adjustments:

Fix PR title spelling
Please use "MinGW" variant (at least in PR title):

% git log --oneline | grep -c MinGW
58
% git log --oneline | grep -c mingw
2

Whenever you are writing a new PR title, please watch out for spelling and naming consistency bugs. They are quite frequent...

The examples given below are just for illustration purposes. They are probably wrong on several levels. Please do not spend time criticizing/discussing them. Again, I do not know enough to write correct examples here and am not going to defend any of the requests below.

In PR description, please mention what this change does at higher/admin level (e.g., "ext_ad_group_acl fails to build (and has to be disabled at ./configure time) without this fix"). Assume mingw scope.
In PR description, please quote a compiler error message (or equivalent) that one would get without these changes. Abridge as needed. If there was no error, rephrase PR description to express the same "now required" idea differently.
In PR description: If possible without heroic efforts, please mention what has changed outside of Squid that prompted these Squid changes (e.g., "newer Windows headers no longer provide legacy APIs that ext_ad_group_acl was using").

rousskov · 2025-02-27T14:49:37Z

src/acl/external/AD_group/ext_ad_group_acl.cc

@@ -181,9 +181,9 @@ Get_primaryGroup(IADs * pUser)
    VariantInit(&var);

    /* Get the primaryGroupID property */
-    hr = pUser->lpVtbl->Get(pUser, L"primaryGroupID", &var);
+    hr = pUser->Get(BSTR(L"primaryGroupID"), &var);


Just for my own edification, what error do you get if this explicit BSTR conversion is removed? Please ignore this question if it takes too long to answer.

Suggested change

hr = pUser->Get(BSTR(L"primaryGroupID"), &var);

hr = pUser->Get(L"primaryGroupID", &var);

This optional question is not requesting any PR changes.

../../../../../src/acl/external/AD_group/ext_ad_group_acl.cc: In function 'HRESULT Recursive_Memberof(IADs*)': ../../../../../src/acl/external/AD_group/ext_ad_group_acl.cc:429:20: error: ISO C++ forbids converting a string constant to 'BSTR' {aka 'wchar_t*'} [-Werror=write-strings] 429 | hr = pObj->Get(L"memberOf", &var); | ^~~~~~~~~~~

Thank you! Using that error, I found an article that, AFAICT, claims that the changes proposed by this PR are buggy: "This code builds (compiles and links) correctly, but it will not function properly because the string does not have a length prefix."

I still do not know enough to be certain that this PR is buggy, but I believe this concern has to be addressed, so I have to block this PR.

kinkie · 2025-02-27T16:13:29Z

Examples of errors this fixes:

../../../../../src/acl/external/AD_group/ext_ad_group_acl.cc:466:39: error: 'IADs' {aka 'struct IADs'} has no member named 'lpVtbl'
  466 |                                 pGrp->lpVtbl->Release(pGrp);
      |                                       ^~~~~~
../../../../../src/acl/external/AD_group/ext_ad_group_acl.cc:468:63: error: 'VARIANT' {aka 'struct tagVARIANT'} has no member named 'n1'
  468 |                                 Group_Path = GetLDAPPath(elem.n1.n2.n3.bstrVal, LDAP_MODE);
      |                                                               ^~

rousskov

Fix PR title spelling

The title was changed, but the above request remains valid. FWIW, I recommend using good text editors for writing PR title/descriptions, but most browsers will also spellcheck text fields for you if you enable that feature for single-line text fields.

rousskov · 2025-02-27T16:39:17Z

src/acl/external/AD_group/ext_ad_group_acl.cc

@@ -181,9 +181,9 @@ Get_primaryGroup(IADs * pUser)
    VariantInit(&var);

    /* Get the primaryGroupID property */
-    hr = pUser->lpVtbl->Get(pUser, L"primaryGroupID", &var);
+    hr = pUser->Get(BSTR(L"primaryGroupID"), &var);


Thank you! Using that error, I found an article that, AFAICT, claims that the changes proposed by this PR are buggy: "This code builds (compiles and links) correctly, but it will not function properly because the string does not have a length prefix."

I still do not know enough to be certain that this PR is buggy, but I believe this concern has to be addressed, so I have to block this PR.

rousskov · 2025-02-27T16:51:12Z

Alex: In PR description, please quote a compiler error message (or equivalent) that one would get without these changes

Francesco: Examples of errors this fixes: ...

I copied that info to the PR description, addressing one of my earlier requests.

mingw: use namleess unions in ext_ad_group_acl

87083ad

squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Feb 27, 2025

Stop leaking BSTRs

48f53e8

squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Feb 27, 2025

rousskov reviewed Feb 27, 2025

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Feb 27, 2025

kinkie changed the title ~~mingw: use namleess unions in ext_ad_group_acl~~ MinGW: use namleess unions in ext_ad_group_acl Feb 27, 2025

kinkie changed the title ~~MinGW: use namleess unions in ext_ad_group_acl~~ MinGW: use namless unions in ext_ad_group_acl Feb 27, 2025

kinkie added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box S-waiting-for-more-reviewers needs a reviewer and/or a second opinion and removed S-waiting-for-author author action is expected (and usually required) labels Feb 27, 2025

rousskov requested changes Feb 27, 2025

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Feb 27, 2025

Use SysAllocString instead of direct BSTR

c2524b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MinGW: use namless unions in ext_ad_group_acl #2004

MinGW: use namless unions in ext_ad_group_acl #2004

kinkie commented Feb 27, 2025 •

edited by rousskov

Loading

rousskov left a comment •

edited

Loading

rousskov Feb 27, 2025

kinkie Feb 27, 2025

rousskov Feb 27, 2025

kinkie commented Feb 27, 2025

rousskov left a comment

rousskov Feb 27, 2025

rousskov commented Feb 27, 2025

	hr = pUser->Get(BSTR(L"primaryGroupID"), &var);
	hr = pUser->Get(L"primaryGroupID", &var);

MinGW: use namless unions in ext_ad_group_acl #2004

Are you sure you want to change the base?

MinGW: use namless unions in ext_ad_group_acl #2004

Conversation

kinkie commented Feb 27, 2025 • edited by rousskov Loading

rousskov left a comment • edited Loading

Choose a reason for hiding this comment

rousskov Feb 27, 2025

Choose a reason for hiding this comment

kinkie Feb 27, 2025

Choose a reason for hiding this comment

rousskov Feb 27, 2025

Choose a reason for hiding this comment

kinkie commented Feb 27, 2025

rousskov left a comment

Choose a reason for hiding this comment

rousskov Feb 27, 2025

Choose a reason for hiding this comment

rousskov commented Feb 27, 2025

kinkie commented Feb 27, 2025 •

edited by rousskov

Loading

rousskov left a comment •

edited

Loading