Skip to content

Fix assigning stack variable to function parameter. #20310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

Quipyowert2
Copy link
Contributor

Fixes #20180 by 1. allocating extra memory for another tempsym_t before the loop with the recursive call to (un)pack_rec, 2. assigning lookahead or savsym to *symptr->previous, which copies the members of that tempsym_t to the tempsym_t pointed to by symptr->previous 3. and finally frees the extra memory after the loop ends but before copying savsym or lookahead to symptr.

This might leak memory if the recursive call to (un)pack_rec croaks, but I'm not sure how to fix that. I suppose I could add a boolean flag to S_unpack_rec and S_pack_rec (or alternatively in tempsym_t) which would be set to false when called from another function and set to true on recursive calls (inside parentheses) and then call Safefree right before every croak in both these functions when isrecursive is true.

@Leont
Copy link
Contributor

Leont commented Sep 17, 2022

I think this would leak if the nested unpack_rec throws an exception

@Quipyowert2
Copy link
Contributor Author

Quipyowert2 commented Sep 18, 2022

Oops, I broke the build. With these changes, \0 and \xa5 are swapped in the output of pack in pack.t lines 1704, 1706 and 1709 which is where it tests marked_upgrade. I don't know why this happens.

@tonycoz
Copy link
Contributor

tonycoz commented Sep 19, 2022

This change as is, is unneeded, I believe the original code is correct and we just need suppressions for cppcheck on those lines.

@Quipyowert2
Copy link
Contributor Author

Sorry about the low quality of this pull request; I am not that familiar with C or Perl internals. I've made a new pull request #20315 which has the changes from the second patch I suggested in #20180.

@tonycoz
Copy link
Contributor

tonycoz commented Sep 19, 2022

Thanks for trying to contribute, I just wish I'd looked over the original issue before you spent time on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address of local auto-variable assigned to a function parameter
3 participants