Fix a potential memory leak in file transfer #126

nordominus · 2019-05-22T06:40:50Z

This potential memory leak in dlt-system-filetransfer.c can happen if only a filename
is passed to dirname(). In the current implementation of dlt-daemon, this issue cannot happen.
But in case of refactoring or different usage there is a chance that it can be triggered.
See: https://linux.die.net/man/3/dirname

The program was tested solely for our own use cases, which might differ from yours.

Andreas Seidl andreas.seidl@daimler.com, Mercedes-Benz AG on behalf of MBition GmbH
https://github.com/Daimler/daimler-foss/blob/master/LEGAL_IMPRINT.md
Licensed under MPL-2.0

gunnarx · 2019-05-22T10:42:25Z

First, thanks for this of course, and this is just some spontaneous feedback (Note, I am not the dlt-daemon maintainer)

Your base branch is a little out of date. Rebasing your patch on GENIVI/master could be done by the maintainer during the merge (since there are no conflicts reported), but it's of course nice if you do it.
The summary line is too long and there is some long and strange signature in the commit message, which looks out of place? You can look at previous discussions in Made writev reliable for fixing data corruption #123 regarding commit message format.

nordominus · 2019-05-22T12:26:20Z

Hi @gunnarx,

many thanks for your feedback!

Yes you're right, I missed to rebase my patch. I'll try to fix that.
I'll modify the summary line and shortned it and added the signed-off signature missing.
But unfortunately I cannot remove the "strange signature" as this are company guidelines and rules I've to follow, when contributing to or communicating with an FOSS project.
I can understand this is an issue and I'll raise this issue internally.

gunnarx · 2019-05-23T16:53:36Z

@nordominus Thanks for your understanding. At first you might perhaps check the following...

Is it really correct you want to put this message into every git commit? Or could it be enough to communicate it differently (such as through this conversation)
Are you aware of any other open-source project that has accepted this text into the git commit history?

Before I go on, let me make clear that the purpose is not to nitpick on what your company lawyers have surely put a lot of work into, but I don't think it is wise for any FOSS project to immediately accept this text into the commit message without first clarifying options. Ultimately, I believe that if every company start crafting their own arbitrary additions then there are various problems with that:

Users of the software would then have any number of additional legal texts to study, which could even be incompatible with each other. There is a reason that projects try to stick to open-source licenses, ideally only one per codebase, and one important reason is that everyone in the whole industry can fully understand and study those licenses once. If every committer start adding their own additions to the license text, it might not be feasible to understand the full legal conditions of the code.
Adding this text to the git commit (if that is truly the intention) will harm developer collaboration. There is no function in git to separate out this boiler plate text from the useful information, so just practically speaking it needs to be placed elsewhere if it is not going to be a serious annoyance to collaborating developers. I think it is ultimately an untenable precedent to do it this way, and would therefore request some more clarification first, if you don't mind.

Now, I don't intend to go into all details here because it is not the time or the place, but here are some of the points that may need to be discussed and fully agreed upon first:

Ensuring everyone understands the existence of the DCO and Sign-Off principle and how it is applied and accepted in the most significant open-source projects that exist.
Even if the DCO text itself is not found to be sufficient by a company, discuss reusing the principle of agreeing on the terms first (compare to DCO) and then simply referring to that agreement (compare to Sign-Off), without the need to repeat it on every commit message.
Confirming whether your words "communicating with an FOSS project" really should mean to put the text into the commit message itself, so that this is not just a misunderstanding? For example, you have provided it also in the PR introduction, which might be enough.
Finally, confirming the existence of very carefully written exclusion of warranty statements that are already provided in the MPL v2.0 license. Certainly, for an untrained eye they look considerably more comprehensive than the text that has been added here about considerations to be made before 'productive use'. For that reason, understanding what the intention behind your company's policy and the text therein, would help the conversation.

And please understand that I'm talking about having a conversation that is needed across all/many open-source projects here - it's not only a small personal view for one particular project only. I may be convinced differently later on, but as I said my feeling is that this policy would be largely untenable for open-source collaboration.

gunnarx

Normally each project under GENIVI GitHub is controlled by the maintainer and in my role as Development Lead for GENIVI Alliance, I usually do not interfere (too much).

But for this I would like to add a formal "Reject" vote at the moment, until we have fully sorted out the implication of introducing the extra text in the git commit message. Thanks to the contributor and everyone else for your understanding.

ssugiura

The former implementation did cause memory leak, but not segfault. Is there any case with segfault?

ssugiura · 2019-05-27T01:57:07Z

src/system/dlt-system-filetransfer.c

-    fdir = dirname(fdir);/*dirname overwrites its argument anyway */
+    char *src_copy = strndup(src, PATH_MAX);
+    MALLOC_ASSERT(src_copy);
+    char* fdir = dirname(src_copy);/*dirname overwrites its argument anyway, depending on argument the returned address might change */


Please keep the line in 80 columns.

Yes it is only a memory leak, which can happen.

Please keep the line in 80 columns.
Mh, line 315 (not touched by me) is even longer. Shall I reformat them too?

Please keep the line in 80 columns.
Mh, line 315 (not touched by me) is even longer. Shall I reformat them too?

Hum, that is also true. But reformatting them is too much and more than what the commit itself you fix, so please modify only your lines. I can do those kind of reformatting for all the files. Thanks for reminding me of this.

Line changed

ssugiura · 2019-06-27T02:10:09Z

@nordominus Is there any update?

nordominus · 2019-07-12T06:37:10Z

@ssugiura Please apologize may late reply. Unfortunately work on both of my pull requests are on hold until the legal concerns are solved (see discussion with @gunnarx). I'm working on it to find a proper solution (@gunnarx thx for your support!), but it will take time.

gunnarx · 2019-07-12T07:56:29Z

Yes, we are working on it :)
Happy to sync up with you again at any time @nordominus

ssugiura · 2019-07-14T23:19:30Z

@nordominus @gunnarx Got it. Thanks for the update!

nordominus · 2019-07-29T11:57:45Z

Hi @gunnarx and @ssugiura quick update on the current status:
We plan to have a solution ready next week for the legal topics, so i can rework the pull requests accordingly. Many thanks for your patience!

ssugiura · 2019-07-29T23:40:44Z

@nordominus Thank you for your update. It's great to hear that. Hope to hear from you soon!

ssugiura · 2019-09-11T05:25:56Z

@nordominus Do you have any update on your work?

nordominus · 2019-09-12T13:45:25Z

@nordominus Do you have any update on your work?

@ssugiura Yes, unfortunately it took longer as expected due to summer vacation time. But I start working on both of my PRs tomorrow and try to get everything fixed for another review

Many thanks for your patience!

ssugiura · 2019-09-12T23:43:38Z

@nordominus Great, thanks for your quick update.

ssugiura · 2019-10-28T00:01:36Z

@nordominus Friendly reminder.

nordominus · 2019-11-07T13:16:41Z

@nordominus Do you have any update on your work?

@ssugiura Yes, unfortunately it took longer as expected due to various reasons.
Many many thanks for your patience and the friendly reminder!

I've reworked the commit as you requested and also changed the commit message and pull request in accordance with my company's contribution guidelines. @gunnarx may you have also a look at it and provide some more feedback? Many thanks!

ssugiura · 2019-11-10T23:36:54Z

@nordominus Thanks a lot for your continuous work! Current one looks much better.
@gunnarx Do you agree with this improvement?

This potential memory leak in dlt-system-filetransfer.c can happen if only a filename is passed to dirname(). In the current implementation of dlt-daemon, this issue cannot happen. But in case of refactoring or different usage there is a chance that it can be triggered. See: https://linux.die.net/man/3/dirname Signed-off-by: Andreas Seidl <andreas.seidl@daimler.com>

Keep comments within 80 columns Signed-off-by: Andreas Seidl <andreas.seidl@daimler.com>

nordominus changed the title ~~Fix a potential segfault/memory leak in dlt-system-filetransfer.c~~ Fix a potential segfault/memory leak in filetransfer May 22, 2019

gunnarx suggested changes May 23, 2019

View reviewed changes

ssugiura suggested changes May 27, 2019

View reviewed changes

nordominus changed the title ~~Fix a potential segfault/memory leak in filetransfer~~ Fix a potential memory leak in file transfer Nov 7, 2019

nordominus force-pushed the dai-fix-filetransfer-segfault branch from 971f2bf to ede4acb Compare November 7, 2019 13:12

Andreas Seidl added 2 commits November 18, 2019 11:09

Fix a potential memory leak in file transfer

eabb3f1

Keep comments within 80 columns Signed-off-by: Andreas Seidl <andreas.seidl@daimler.com>

nordominus force-pushed the dai-fix-filetransfer-segfault branch from ede4acb to eabb3f1 Compare November 18, 2019 11:32

ssugiura merged commit 346781f into COVESA:master Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a potential memory leak in file transfer #126

Fix a potential memory leak in file transfer #126

nordominus commented May 22, 2019 •

edited

Loading

gunnarx commented May 22, 2019 •

edited

Loading

nordominus commented May 22, 2019

gunnarx commented May 23, 2019 •

edited

Loading

gunnarx left a comment

ssugiura left a comment

ssugiura May 27, 2019

nordominus Nov 7, 2019

nordominus Nov 7, 2019

ssugiura Nov 10, 2019

nordominus Nov 18, 2019

ssugiura commented Jun 27, 2019

nordominus commented Jul 12, 2019

gunnarx commented Jul 12, 2019

ssugiura commented Jul 14, 2019

nordominus commented Jul 29, 2019

ssugiura commented Jul 29, 2019

ssugiura commented Sep 11, 2019

nordominus commented Sep 12, 2019

ssugiura commented Sep 12, 2019

ssugiura commented Oct 28, 2019

nordominus commented Nov 7, 2019 •

edited

Loading

ssugiura commented Nov 10, 2019

Fix a potential memory leak in file transfer #126

Fix a potential memory leak in file transfer #126

Conversation

nordominus commented May 22, 2019 • edited Loading

gunnarx commented May 22, 2019 • edited Loading

nordominus commented May 22, 2019

gunnarx commented May 23, 2019 • edited Loading

gunnarx left a comment

Choose a reason for hiding this comment

ssugiura left a comment

Choose a reason for hiding this comment

ssugiura May 27, 2019

Choose a reason for hiding this comment

nordominus Nov 7, 2019

Choose a reason for hiding this comment

nordominus Nov 7, 2019

Choose a reason for hiding this comment

ssugiura Nov 10, 2019

Choose a reason for hiding this comment

nordominus Nov 18, 2019

Choose a reason for hiding this comment

ssugiura commented Jun 27, 2019

nordominus commented Jul 12, 2019

gunnarx commented Jul 12, 2019

ssugiura commented Jul 14, 2019

nordominus commented Jul 29, 2019

ssugiura commented Jul 29, 2019

ssugiura commented Sep 11, 2019

nordominus commented Sep 12, 2019

ssugiura commented Sep 12, 2019

ssugiura commented Oct 28, 2019

nordominus commented Nov 7, 2019 • edited Loading

ssugiura commented Nov 10, 2019

nordominus commented May 22, 2019 •

edited

Loading

gunnarx commented May 22, 2019 •

edited

Loading

gunnarx commented May 23, 2019 •

edited

Loading

nordominus commented Nov 7, 2019 •

edited

Loading