Skip to content

Fix invalid robot names#5959

Merged
ad-daniel merged 42 commits intodevelopfrom
fix-invalid-robot-names
Mar 10, 2023
Merged

Fix invalid robot names#5959
ad-daniel merged 42 commits intodevelopfrom
fix-invalid-robot-names

Conversation

@ad-daniel
Copy link
Copy Markdown
Contributor

@ad-daniel ad-daniel commented Mar 1, 2023 •
edited
Loading

Description

Fixes #5452

This PR:

  • If the percentage-encoded robot name length is beyond an arbitrary size of 70 characters, a hash is used instead. The risk is that if the Robot name contains non-ASCII characters, the percentage encoding can result in a huge string which ends up crashing the controller when the connection takes place (QLocalServer has a limit of 106 characters for the server name)
  • Reverts Fixed tmp path on macOS #5484 so that "/tmp" is used instead of TMP_DIR and changes the permissions so that multiple users can

@ad-daniel ad-daniel added test suite Start the test suite test sources Start the sources test on all platforms labels Mar 1, 2023
@ad-daniel ad-daniel added this to the R2023b milestone Mar 1, 2023
@ad-daniel ad-daniel self-assigned this Mar 1, 2023
@ad-daniel ad-daniel removed the test suite Start the test suite label Mar 9, 2023
@ad-daniel
Copy link
Copy Markdown
Contributor Author

ad-daniel commented Mar 10, 2023

I've tested it on linux and macos, couldn't test on windows

@ad-daniel ad-daniel marked this pull request as ready for review March 10, 2023 07:23
@ad-daniel ad-daniel requested a review from a team as a code owner March 10, 2023 07:23
@omichel omichel self-requested a review March 10, 2023 07:39
omichel
omichel requested changes Mar 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@omichel omichel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It looks very good to me.
I am now testing on Windows different use-cases, including local and remote controllers.

Comment thread src/webots/control/WbController.cpp Outdated
Comment thread src/webots/control/WbController.cpp Outdated
Comment thread src/webots/control/WbController.cpp Outdated
Comment thread src/webots/core/WbStandardPaths.cpp Outdated
Comment thread src/webots/core/WbTemplateEngine.cpp Outdated
ad-daniel and others added 6 commits March 10, 2023 10:14
@ad-daniel @omichel
Update src/webots/control/WbController.cpp
30d4c45 
Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
@ad-daniel @omichel
Update src/webots/control/WbController.cpp
4a741ec 
Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
@ad-daniel @omichel
Update src/webots/control/WbController.cpp
430298e 
Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
@ad-daniel
Review fixes
2e7c00a
@ad-daniel
Add fixme note
581bc88
@omichel
Fixed behavior on Windows
3694740
@omichel omichel self-requested a review March 10, 2023 09:42
omichel
omichel previously approved these changes Mar 10, 2023
View reviewed changes
@omichel
Copy link
Copy Markdown
Member

omichel commented Mar 10, 2023

I would also add a line in the change log.

@omichel omichel self-requested a review March 10, 2023 10:14
@omichel omichel dismissed their stale review March 10, 2023 10:14

I would also add a line in the change log

omichel
omichel approved these changes Mar 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

Thank you.

@ad-daniel ad-daniel merged commit a302265 into develop Mar 10, 2023
@ad-daniel ad-daniel deleted the fix-invalid-robot-names branch March 10, 2023 13:24
omichel
omichel reviewed Apr 13, 2023
View reviewed changes
Comment thread src/webots/control/WbController.cpp
Comment on lines +324 to +326

// FIXME: from Qt 6.4 onwards, QDir::mkdir can be used to set the permissions
QProcess::execute("sh", QStringList() << "-c" << QString("chmod -R 777 %1").arg(mIpcPath));
Copy link
Copy Markdown
Member

@omichel omichel Apr 13, 2023

Choose a reason for hiding this comment

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

This code seems wrong: useless and not working on Windows where it is supposed to be executed.

@omichel omichel mentioned this pull request Apr 13, 2023
Merged
@brettle brettle mentioned this pull request Aug 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@omichel omichel omichel approved these changes

Assignees

@ad-daniel ad-daniel

Labels

test sources Start the sources test on all platforms

Milestone

R2023b

Development

Successfully merging this pull request may close these issues.

2 participants

@ad-daniel @omichel