Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add rlm_python3 config parameter 'utf8_fail_as_bytes' #8

Closed
wants to merge 5 commits into from
Closed

Add rlm_python3 config parameter 'utf8_fail_as_bytes' #8

wants to merge 5 commits into from

Conversation

aren
Copy link

@aren aren commented May 25, 2023

Description of the change

Add rlm_python3 config parameter 'utf8_fail_as_bytes' which will return a byte string if it a string can't be interpreted as UTF8.

Type of change

  • Bug fix
  • New feature

Testing

  • Testing information has been added - test cases checklist and steps followed for testing.
  • Testing screenshot(s) attached for more information.

Security

  • This PR has security related considerations.

@aren
Add rlm_python3 config parameter 'utf8_fail_as_bytes' which will retu…
0f3a0b6 
…rn a byte string if it a string can't be interpreted as UTF8.
@aren aren requested a review from larrysun May 25, 2023 21:43
aren
aren commented May 26, 2023
View reviewed changes
raddb/mods-available/python3 Outdated
@@ -27,6 +27,10 @@ python3 {
# This option prevales over "pass_all_vps"
# pass_all_vps_dict = no

# Added by Foxpass:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make these lines start with tab so they match the others

@larrysun
Copy link

Fixed compilation errors and tested in foxpass master branch.

aren
aren commented May 26, 2023
View reviewed changes
raddb/mods-available/python3 Outdated
@@ -27,6 +27,10 @@ python3 {
# This option prevales over "pass_all_vps"
# pass_all_vps_dict = no

# Added by Foxpass:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line, because we don't want this when we do a PR for upstream

raddb/mods-available/python3 Outdated Show resolved Hide resolved
larrysun
larrysun approved these changes May 26, 2023
View reviewed changes
Copy link

@larrysun larrysun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in master branch of foxpass. For username with invalid utf-8 string, got:

  • "python_error_log:210, Exception type: <class 'SystemError'>, Exception value: <class 'UnicodeDecodeError'> returned a result with an error set" - when option set to "no"
  • "Conversion to Unicode failed, returning User-Name as bytes" - when option set to "yes"

asakapab0i pushed a commit that referenced this pull request Jul 27, 2023
@jpereira
Fix memory-leak in "radiusd -XCM" (FreeRADIUS#4934)
260d210 
Such error:

Current state of talloced memory:
full talloc report on 'null_context' (total      0 bytes in   1 blocks)

=================================================================
==85543==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 96 byte(s) in 1 object(s) allocated from:
    #0 0x5598fcd87f3e in malloc (/home/jpereira/Devel/FreeRADIUS/freeradius-server-v3.2.x.git-linux/build/bin/local/radiusd+0x20cf3e) (BuildId: 3bf5bfb4fd72e1e1112726414556f8a4f339789f)
    #1 0x7f1cc4453d7f in __talloc_with_prefix /build/talloc-NvEq5A/talloc-2.3.3/bin/default/../../talloc.c:783:9
    #2 0x7f1cc4455a5d in __talloc /build/talloc-NvEq5A/talloc-2.3.3/bin/default/../../talloc.c:825:9
    #3 0x7f1cc4455a5d in _talloc_named_const /build/talloc-NvEq5A/talloc-2.3.3/bin/default/../../talloc.c:982:8
    #4 0x7f1cc4455a5d in talloc_enable_null_tracking /build/talloc-NvEq5A/talloc-2.3.3/bin/default/../../talloc.c:2353:18
    #5 0x7f1cc4455a5d in talloc_enable_null_tracking /build/talloc-NvEq5A/talloc-2.3.3/bin/default/../../talloc.c:2350:15
    #6 0x5598fceb65b1 in main /home/jpereira/Devel/FreeRADIUS/freeradius-server-v3.2.x.git-linux/src/main/radiusd.c:313:3
    #7 0x7f1cc342350f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #8 0x7f1cc34235c8 in __libc_start_main csu/../csu/libc-start.c:381:3
    #9 0x5598fcd02514 in _start (/home/jpereira/Devel/FreeRADIUS/freeradius-server-v3.2.x.git-linux/build/bin/local/radiusd+0x187514) (BuildId: 3bf5bfb4fd72e1e1112726414556f8a4f339789f)

SUMMARY: AddressSanitizer: 96 byte(s) leaked in 1 allocation(s).
asakapab0i pushed a commit that referenced this pull request Jul 27, 2023
@jpereira @alandekok
Fix memory leak in client_add()
8ec1f31 
Such error when we run: radiusd -CX

Configuration appears to be OK
Allocated memory at time of report:
Current state of talloced memory:
full talloc report on 'null_context' (total   1057 bytes in   4 blocks)
    autofree_context               contains      1 bytes in   2 blocks (ref 0) 0x608000000400
        bool                           contains      1 bytes in   1 blocks (ref 0) 0x60b000044a90
    RADCLIENT_LIST                 contains   1056 bytes in   1 blocks (ref 0) 0x6190000032e0

=================================================================
==43730==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1152 byte(s) in 1 object(s) allocated from:
    #0 0x5603d1a170be in malloc (/__w/freeradius-server/freeradius-server/build/bin/local/radiusd+0x20b0be) (BuildId: acbe3a0941626cf6f01ae6b2c12df877fb8fc009)
    #1 0x7f2c07e40c86 in _talloc_zero (/lib/x86_64-linux-gnu/libtalloc.so.2+0x6c86) (BuildId: f3c1074a602981acb4683b4df6b7733b104ba7d4)
    #2 0x5603d1a62780 in client_list_init (/__w/freeradius-server/freeradius-server/build/bin/local/radiusd+0x256780) (BuildId: acbe3a0941626cf6f01ae6b2c12df877fb8fc009)
    #3 0x5603d1a62a4a in client_add (/__w/freeradius-server/freeradius-server/build/bin/local/radiusd+0x256a4a) (BuildId: acbe3a0941626cf6f01ae6b2c12df877fb8fc009)
    #4 0x5603d1a6b713 in client_list_parse_section (/__w/freeradius-server/freeradius-server/build/bin/local/radiusd+0x25f713) (BuildId: acbe3a0941626cf6f01ae6b2c12df877fb8fc009)
    #5 0x5603d1ae8532 in main_config_init (/__w/freeradius-server/freeradius-server/build/bin/local/radiusd+0x2dc532) (BuildId: acbe3a0941626cf6f01ae6b2c12df877fb8fc009)
    #6 0x5603d1b45bc4 in main (/__w/freeradius-server/freeradius-server/build/bin/local/radiusd+0x339bc4) (BuildId: acbe3a0941626cf6f01ae6b2c12df877fb8fc009)
    #7 0x7f2c07981082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #8 0x5603d19924fd in _start (/__w/freeradius-server/freeradius-server/build/bin/local/radiusd+0x1864fd) (BuildId: acbe3a0941626cf6f01ae6b2c12df877fb8fc009)

SUMMARY: AddressSanitizer: 1152 byte(s) leaked in 1 allocation(s).
@asakapab0i
Copy link

Hi @aren , can we merge this? It seems to fix the compilation issues.

@aren
Copy link
Author

aren commented Sep 1, 2023

yes

@asakapab0i
Copy link

This is not required as we've already cherry-picked this commit to the target branch

@asakapab0i asakapab0i closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larrysun larrysun larrysun approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aren @larrysun @asakapab0i