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.

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

Bring iniparser library codebase in line with current version 4.1 #271

Merged
merged 1 commit into from
Apr 6, 2023
Merged

Bring iniparser library codebase in line with current version 4.1 #271

merged 1 commit into from
Apr 6, 2023

Conversation

dgsga
Copy link
Contributor

@dgsga dgsga commented Apr 5, 2023

This commit removes deprecated strcpy calls from iniparser and fixes the associated buffer overflow causing afpd to crash when compiled with clang.

@rdmark
Copy link
Member

rdmark commented Apr 5, 2023

@dgsga Thanks for another round of PRs! I appreciate you taking the time to port over improvements from your fork.

I'm not too familiar with iniparser, but I assume this is autogenerated code here in this PR? Just for my future reference, how was it generated?

@dgsga
Copy link
Contributor Author

dgsga commented Apr 5, 2023

The comments explaining each function are taken verbatim from @ndevilla's iniparser repo on GitHub. Each function was adapted from this repo after comparing the Netatalk iniparser implementation with the original v3 of iniparser.

@dgsga
Copy link
Contributor Author

dgsga commented Apr 5, 2023

@rdmark, I have been thinking about creating a big PR that would bring compatibility with modern Intel / Apple Silicon macOS hosts to the upstream master. What do you think??

@rdmark
Copy link
Member

rdmark commented Apr 5, 2023

The comments explaining each function are taken verbatim from @ndevilla's iniparser repo on GitHub. Each function was adapted from this repo after comparing the Netatalk iniparser implementation with the original v3 of iniparser.

That makes sense. I realized only now when looking at the commit history that iniparser is a 3rd party library originally. With iniparser being distributed under the MIT license, I wonder if a copyright notice is apt for these source files? Our own COPYING only has GPL2, so perhaps adding this to the header of all iniparser source files is appropriate: https://github.com/ndevilla/iniparser/blob/master/LICENSE

IANAL, but this is my takeaway from a few minutes of researching online what to do when shipping MIT licensed code in a project distributed under the GPL.

BTW, I assume this changeset is proven in your fork? The iniparser readme mentions breaking API changes between v3 and v4.

@rdmark
Copy link
Member

rdmark commented Apr 5, 2023

I have been thinking about creating a big PR that would bring compatibility with modern Intel / Apple Silicon macOS hosts to the upstream master. What do you think??

I would personally be all for that! Would be handy to run netatalk3 on my M1 MBP (though this is already possible with your fork of course.) Would you mind raising a github ticket for this for tracking?

This commit removes deprecated strcpy calls from iniparser and fixes the buffer overflow causing afpd to crash when compiled with clang.

Add missing MIT license to iniparser files
@dgsga
Copy link
Contributor Author

dgsga commented Apr 6, 2023

The comments explaining each function are taken verbatim from @ndevilla's iniparser repo on GitHub. Each function was adapted from this repo after comparing the Netatalk iniparser implementation with the original v3 of iniparser.

That makes sense. I realized only now when looking at the commit history that iniparser is a 3rd party library originally. With iniparser being distributed under the MIT license, I wonder if a copyright notice is apt for these source files? Our own COPYING only has GPL2, so perhaps adding this to the header of all iniparser source files is appropriate: https://github.com/ndevilla/iniparser/blob/master/LICENSE

IANAL, but this is my takeaway from a few minutes of researching online what to do when shipping MIT licensed code in a project distributed under the GPL.

BTW, I assume this changeset is proven in your fork? The iniparser readme mentions breaking API changes between v3 and v4.

I agree, the license needs to be added as the original code in @ndevilla's repo is released under its terms. Quoting from the license itself:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

So license is added, force-pushed to PR. I have been using the basis of this patch for 2 years on my fork without issue. I recently updated it to add the function explanations and a small amount of missing buffer overflow check code that is present in @ndevilla's repo

@dgsga
Copy link
Contributor Author

dgsga commented Apr 6, 2023

I have been thinking about creating a big PR that would bring compatibility with modern Intel / Apple Silicon macOS hosts to the upstream master. What do you think??

I would personally be all for that! Would be handy to run netatalk3 on my M1 MBP (though this is already possible with your fork of course.) Would you mind raising a github ticket for this for tracking?

Ticket added.

@rdmark rdmark merged commit b568955 into Netatalk:main Apr 6, 2023
1 check passed
@rdmark
Copy link
Member

rdmark commented Apr 6, 2023

Perfect, thank you!

@rdmark
Copy link
Member

rdmark commented Apr 6, 2023

FWIW gcc10 throws a few warnings with the new code that weren't there before:

make[3]: Entering directory '/home/dmark/dev/netatalk/libatalk/iniparser'
  CC       dictionary.lo
  CC       iniparser.lo
iniparser.c: In function ‘atalk_iniparser_line’:
iniparser.c:546:27: warning: passing argument 1 of ‘strstrip’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  546 |     strcpy(line, strstrip(input_line));
      |                           ^~~~~~~~~~
iniparser.c:74:31: note: expected ‘char *’ but argument is of type ‘const char *’
   74 | static char * strstrip(char * s)
      |                        ~~~~~~~^
In function ‘atalk_iniparser_line’,
    inlined from ‘atalk_iniparser_load’ at iniparser.c:678:17:
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
  597 |     free(line);
      |     ^~~~~~~~~~
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
iniparser.c:597:5: warning: attempt to free a non-heap object ‘line’ [-Wfree-nonheap-object]
  CCLD     libiniparser.la
make[3]: Leaving directory '/home/dmark/dev/netatalk/libatalk/iniparser'

@dgsga dgsga deleted the prtest branch April 6, 2023 18:28
@dgsga
Copy link
Contributor Author

dgsga commented Apr 6, 2023

@rdmark, thanks for flagging this. I spotted in your log that line 546 of iniparser.c contains a call to strcpy that should not be there! Checked my files and it appears that I made an error in how I created the PR. Please see the latest PR for the fix. Let me know if you would like to revert the previous commit and then apply a fresh PR or are happy just to apply the fix. Apologies!

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.

None yet

2 participants